Skip to content
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

Merged
merged 7 commits into from
Mar 20, 2019
Merged

Conversation

2color
Copy link
Contributor

@2color 2color commented Mar 18, 2019

What

  • Add rxjs as a dependency
  • Improve dev experience by watching the script in dev/watch mode too
  • Make rxjs v6 code changes where necessary

This PR is in the same vein as #698

@2color 2color marked this pull request as ready for review March 18, 2019 15:29
@2color 2color requested review from AquiGorka, sohkai and bpierre March 18, 2019 15:29
@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.915% when pulling 2fb8fee on rxjs6-upgrade-token-manager into d0896d4 on master.

Copy link
Contributor

@bpierre bpierre left a 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 ✌️

@@ -232,9 +220,6 @@ function loadTokenSettings(token) {

function loadTokenSymbol(token) {
Copy link
Contributor

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().

Copy link
Contributor

@sohkai sohkai left a 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.

@2color 2color force-pushed the rxjs6-upgrade-token-manager branch from 949a0a0 to 1fabae5 Compare March 20, 2019 10:01
@2color
Copy link
Contributor Author

2color commented Mar 20, 2019

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)

For sure.

@2color 2color merged commit d6ea81c into master Mar 20, 2019
@2color 2color deleted the rxjs6-upgrade-token-manager branch March 20, 2019 10:05
MickdeGraaf pushed a commit to MickdeGraaf/aragon-apps that referenced this pull request Jan 28, 2020
* 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
ramilexe pushed a commit to ConsiderItDone/aragon-apps that referenced this pull request Dec 9, 2021
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants