Myriad of changes #9
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Also make the event names relevant. finalFailure is not the finalFailure for the simringer as a whole, also added data for the events finalSuccess will tell you which uri was successful failure will tell you the err and the uri in an obj
designed so that the simringer no longer handles its own timesouts, and no longer tracks failures as a way of rejecting the final promise. Pass in timeout as null or false, and listen for the relevant events and decide on when you want to terminate the simringer yourself
other code in _attemptOne calls killCalls and then removes a uri from the Map afterwards This proves that you expect the URI to still be in the map. Instead of clearing (because one could still be in there), delete them individually
davehorton
reviewed
Nov 18, 2019
| @@ -47,7 +49,7 @@ function transfer(opts) { | |||
| return referHandler.transfer(); | |||
| } | |||
|
|
|||
| class Simring { | |||
| class Simring extends Emitter{ | |||
| constructor(req, res, uriList, opts, notifiers) { | |||
| const callOpts = { | |||
There was a problem hiding this comment.
don't you need to call super() as the first thing in the constructor?
There was a problem hiding this comment.
you're probably 100% right here :) Like I said - completely non tested. I'll go test the changes myself this week and we can go from there
|
It looks pretty good to me from a quick review, but my main thought is that I really need to create a comprehensive test suite for this package because things are getting complex and probably now is the time to create a 100% test suite that we can update going forward. Otherwise, I'm not sure we can have confidence that future changes aren't breaking things. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Allow Simringer class users access to events
Also make the event names relevant. finalFailure is not the finalFailure for the simringer as a whole, Also added data for the events. finalSuccess will tell you which uri was successful. failure will tell you the err and the uri in an obj
allow external application to cancel simringer
designed so that the simringer no longer handles its own timesouts, and no longer tracks failures
as a way of rejecting the final promise. Pass in timeout as null or false, and listen for the relevant events and decide on when you want to terminate the simringer yourself
dont clear whole map, delete each uri
other code in _attemptOne calls killCalls and then removes a uri from the Map afterwards
This proves that you expect the URI to still be in the map. Instead of clearing (because one could still be in there), delete them individually
Also some whitespace changes to follow the conventions in the file
Currently untested so submitting as a draft to see what you think of the changes