-
Notifications
You must be signed in to change notification settings - Fork 212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade rxjs to v6 in token-manager app #716
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯💯💯
I left several comments but they are all about the same thing: using toPromise()
earlier ✌️
apps/token-manager/app/src/script.js
Outdated
@@ -232,9 +220,6 @@ function loadTokenSettings(token) { | |||
|
|||
function loadTokenSymbol(token) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And similar to the other comments, we could probably remove loadTokenSymbol()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 @2color Do you think it would make sense if we just changed all the single-emission observables in @aragon/api
to return promises instead and release a new major version of it (v2.0.0
) (aragon/aragon.js#264)? Then we can remove all the toPromise()
and just use them natively as promises.
- remove first/take where the observable returns a single value and completes because of sendAndObserveResponse
949a0a0
to
1fabae5
Compare
For sure. |
* Import new package name * Add rxjs as an explicit dependency * Watch background script too * fixup! Watch background script too * Migrate script to use rxjs6 - remove first/take where the observable returns a single value and completes because of sendAndObserveResponse * Migrate app to use rxjs v6 * Use toPromise wherever possible
* Import new package name * Add rxjs as an explicit dependency * Watch background script too * fixup! Watch background script too * Migrate script to use rxjs6 - remove first/take where the observable returns a single value and completes because of sendAndObserveResponse * Migrate app to use rxjs v6 * Use toPromise wherever possible
What
rxjs
as a dependencyThis PR is in the same vein as #698