previousFetchTime parameter for FETCH_SYNC_RECORDS #363
Conversation
|
CI fails because needs rebase. |
AlexeyBarabash
force-pushed
the
poll_sqs_after24h
branch
2 times, most recently
from
7904db5
to
1310f20
Compare
AlexeyBarabash
requested review from
darkdh,
SergeyZhukovsky,
samartnik and
bridiver
client/constants/messages.js
Outdated
| @@ -57,7 +57,7 @@ const messages = { | |||
| * with new records, do | |||
| * GET_EXISTING_OBJECTS -> RESOLVE_SYNC_RECORDS -> RESOLVED_SYNC_RECORDS | |||
| */ | |||
| FETCH_SYNC_RECORDS: _, /* @param Array.<string> categoryNames, @param {number} startAt (in seconds or milliseconds), @param {number=} maxRecords limit response to a given max number of records. set to 0 or falsey to not limit the response */ | |||
| FETCH_SYNC_RECORDS: _, /* @param Array.<string> categoryNames, @param {number} startAt (in seconds or milliseconds), @param {number=} maxRecords limit response to a given max number of records. set to 0 or falsey to not limit the response, @param {number} previousFetchTime (in seconds or milliseconds) */ | |||
There was a problem hiding this comment.
not specifying whether the value is seconds or milliseconds seems like a really bad idea to me, what is the reason for this?
There was a problem hiding this comment.
@bridiver , seconds or milliseconds as a requirement of output were introduced long time ago , 349592a#diff-e10a265c86f058932ac016e27462b9a5R54 , but we don't know what was the reason.
There was a problem hiding this comment.
That's fine for existing params, but I don't think we should continue the practice for new ones. I think it's an extremely bad idea to pass a timestamp that doesn't specify one of the other
.gitignore
Outdated
| bundles/*.js | ||
| bundles/constants/*.js |
There was a problem hiding this comment.
just fyi - you can do `bundles/**/*.js and that will cover all subdirectories (and the main directory) if that's what you want
AlexeyBarabash
force-pushed
the
poll_sqs_after24h
branch
from
847f24d
to
e36acfa
Compare
|
Squashed some "fix" commits and done force push. |
previousFetchTime parameter for FETCH_SYNC_RECORDS
Related brave-browser issue brave/brave-browser#7327 .
Related brave-core PR brave/brave-core#4231 .