feat(2883): Error handling improvements#2
Conversation
islathehut
force-pushed
the
feat/error-handling-improvements
branch
from
89860d2 to
e5ca22a
Compare
adrastaea
left a comment
adrastaea
left a comment
There was a problem hiding this comment.
These changes cause a number of tests in the repo to fail which means that we're likely breaking some fundamental assumptions that OrbitDB is making. When I reverted the changes to sync.js, those failures largely resolved.
| meta = meta || {} | ||
| referencesCount = Number(referencesCount) > -1 ? referencesCount : defaultReferencesCount | ||
|
|
||
| events = events || new EventEmitter() |
There was a problem hiding this comment.
Why do we care about passing in our own event emitter?
There was a problem hiding this comment.
This was just so we could share event emitters between different elements (e.g. storage and database instance).
| } | ||
| return value | ||
|
|
||
| return undefined |
There was a problem hiding this comment.
I'm concerned that this changes the expected behavior of the function by not re-emitting the error.
There was a problem hiding this comment.
Ah so like if an update error occurs we wouldn't emit the get error?
There was a problem hiding this comment.
Yeah, suppose that the sync protocol depends on the error being emitted when a block can't be gotten to abort the stream, but we suppress the error and then the stream continues erroneously. On the other hand, what you did here seems to match the behavior in the included level.js, so I'm going to approve.
Turns out I accidentally moved a piece of code into a for loop. I think it worked fine in practice because we were always executing that for loop but in tests we weren't. Moving it back out of the loop fixed all of the tests. |
|
I made some changes to tests to push all of the test data (with the exception of tests that specifically use all default parameters) into the test directory so cancelling tests or bad cleanup won't leave a bunch of junk in git. I'm not sure why the linter is complaining because the comma its referencing doesn't exist 🤷♀️ |
islathehut
changed the title
islathehut
changed the title
2 participants
No description provided.