Skip to content
This repository has been archived by the owner on Jan 20, 2020. It is now read-only.

Fix UnhandledPromiseRejectionWarning in OrderbookSync #239

Merged
merged 2 commits into from
Jan 20, 2018
Merged

Fix UnhandledPromiseRejectionWarning in OrderbookSync #239

merged 2 commits into from
Jan 20, 2018

Conversation

rmm5t
Copy link
Contributor

@rmm5t rmm5t commented Jan 19, 2018 •
edited
Loading

I got tired of seeing the following warning in the test suite:

(node:59176) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 3): Error: Failed to load orderbook: whoops

Turned out that simplifying the code behind grabbing the initial Orderbook did the trick.

I think this fixes #233

/cc @fb55

@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 19, 2018 •
edited
Loading

(Please hold off on reviewing this until I comment again. I realized there is more to this that I might need to refactor.)

@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 19, 2018

Turned out that a full refactor and simplification of OrderbookSync and associated error-related tests were necessary to eliminate the UnhandledPromiseRejectionWarning for good.

/cc @fb55

@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 19, 2018 •
edited
Loading

More fixes to the tests were just added. It turned out that many of the underlying tests for OrderbookSync were mostly bullocks.

Btw, I realized most of these issues while trying to upgrade the ws and mocha dependencies. Doing so exposed a bunch of race conditions.

@rmm5t rmm5t force-pushed the fix-unhandled-promise-rejection-warning branch from af6efa5 to 43b0a77 Compare January 19, 2018 21:53
@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 19, 2018 •
edited
Loading

Btw, I think this fixes #233.

@rmm5t rmm5t force-pushed the fix-unhandled-promise-rejection-warning branch from 43b0a77 to 1a4d760 Compare January 19, 2018 22:14
@fb55
Copy link
Contributor

fb55 commented Jan 19, 2018

This looks great. You okay with merging this, or are there any changes left you'd like to make?

@rmm5t
Copy link
Contributor Author

rmm5t commented Jan 20, 2018

Please merge. I actually need this merged before I can work on further fixes and upgrades.

@fb55 fb55 merged commit 20d1292 into coinbase:master Jan 20, 2018
@fb55
Copy link
Contributor

fb55 commented Jan 20, 2018

Sure thing — great work!

@rmm5t rmm5t mentioned this pull request Jan 20, 2018
Merged
@rmm5t rmm5t deleted the fix-unhandled-promise-rejection-warning branch January 20, 2018 01:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

OrderbookSync throws errors - Response could not be parsed as JSON
2 participants
@rmm5t @fb55