Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign uphandle sync PUT errors #150
Conversation
|
Tested on I'm not sure if this will solve the original error, but it should help with debugging. |
|
I'm new to this code, but I wonder if it would make sense to check here too? |
|
@bsclifton that sounds reasonable – maybe |
|
@bsclifton i think if record is falsey it should throw an error because that shouldn't happen. in that case, the caller catches the error and it's a no-op anyway. |
|
also if record is falsey, i think https://github.com/brave/sync/pull/150/files#diff-6ce8437e6c27185736d22013ce441e15R135 will throw an error and bufferedPut won't be called in the first place |
| @@ -131,14 +131,20 @@ const startSync = (requester) => { | |||
| logSync(`Sending ${records.length} records`) | |||
| const categoryId = proto.categories[category] | |||
| records.forEach((record) => { | |||
| if (!record) { | |||
| return | |||
This comment has been minimized.
This comment has been minimized.
ayumi
Jul 18, 2017
Contributor
should we be logging a warning here? falsey record is not to be expected
and check that record is non-empty
|
the issue @bsclifton experienced seems to be due to our version of protobufjs not accepting |
|
working on test brokenness |
| if (data.fields && data.fields.length > 0) { | ||
| object[type] = pickFields(data, data.fields) | ||
| // Empty the fields field for backwards compatibility with protobufjs 6.6 |
This comment has been minimized.
This comment has been minimized.
diracdeltas
Jul 18, 2017
Author
Member
@ayumi is this needed? the fields field seems to always be empty with protobufjs 6.6 but it is populated with the correct fields in 6.8. however, it is expected to be empty for the tests to pass.
This comment has been minimized.
This comment has been minimized.
ayumi
Jul 18, 2017
Contributor
i don't remember the details. what i do remember is pbjs didn't distinguish between props with unset default value versus fields explicitly set with that default value
so I added fields to explicitly track which fields were set.
|
This worked perfectly! |
diracdeltas commentedJul 17, 2017
fix #149