Connection manager replacement #573
Conversation
Codecov Report
@@ Coverage Diff @@
## master #573 +/- ##
==========================================
- Coverage 95.87% 95.76% -0.12%
==========================================
Files 87 88 +1
Lines 1988 2196 +208
Branches 276 305 +29
==========================================
+ Hits 1906 2103 +197
- Misses 45 51 +6
- Partials 37 42 +5
Continue to review full report at Codecov.
|
| ngOnInit() {} | ||
| ngOnInit() { | ||
| this.form.controls.token.valueChanges | ||
| .pipe(takeUntil(this.ngUnsubscribe)) |
There was a problem hiding this comment.
Nitpick: this takeUntil(subject) is a little bit weird, if the subject is created only for cleanup. A more streamlined way would be to save subscriptions, and then .add multiple if needed and .unsubscribe() at the end. If you do that because you need complete callbacks for cleanup, check the finalize operator.
There was a problem hiding this comment.
We had some problem in tests with the old add() - unsubscribe() pattern. I don't remember the exact reason, but was something about the mocked observables. That's why I moved to the takeUntil() approach. Seems also to be recommended for Angular: https://stackoverflow.com/questions/38008334/angular-rxjs-when-should-i-unsubscribe-from-subscription
| } | ||
|
|
||
| ngOnInit(): void { | ||
| this.subscribeToSuggestions(); |
There was a problem hiding this comment.
Nitpick: private methods with internal, explicit subscriptions aren't usually the recommended way to do that in Angular. Maybe you could just initialize private cold observables, possibly shared, and then use | async on template, which does handle subscriptions automatically. Declarative is usually better than imperative.
There was a problem hiding this comment.
Makes totally sense! I like it your way better. Thanks!
There was a problem hiding this comment.
Couldn't make it because there were too many changes needed in the tests. Not worth the effort.
| @@ -1,7 +1,7 @@ | |||
| import BigNumber from 'bignumber.js'; | |||
|
|
|||
| export function losslessParse(json: string): any { | |||
| const numberRegex = /^\d+$/; | |||
| const numberRegex = /^\d+(\.\d+)?$/; | |||
There was a problem hiding this comment.
This losslessParse may work only if the BNs are stringified on the JSONs (since this revive function from JSON.parse is called only after the number already got parsed as such), while it seems the current PFS implementation encodes the capacity and other BNs as JSON number. This may only be working now if the numbers on your test were small (i.e. < 2^53).
We had this issue on the LC, and ended up using json-bigint lib to parse these JSONs. Check here for our implementation.
There was a problem hiding this comment.
Notice you need to use a proper decoder/schema validator if you go the way we did, since it'll ensure these values are kept as strings, or use it with your reviver function after using the lib to ensure the BNs are kept as strings or bigint or whatever.. We already had the schemas in place, so went with it, but you can use the reviver as well if you don't care that much with the typesafety there.
There was a problem hiding this comment.
As we don't use the numbers from the PFS API, I won't fix it now. I have put it into this issue: #578
Thanks for finding this!
src/app/services/raiden.service.ts
Outdated
| let notificationIdentifier: number; | ||
| let errorCount = 0; | ||
|
|
||
| return of(null).pipe( |
There was a problem hiding this comment.
Nitpick: you could replace of(constant)+tap with a defer call, if it's just to ensure a function is called at subscription time.
There was a problem hiding this comment.
Nice! I didn't know about it.
There was a problem hiding this comment.
some nitpicks here and there, but this is just too big to review line-by-line. Anyway, seems solid to me, UX on specific edge cases like the slider distributor may only be possible to be tested manually, but if it's good for you, it LGTM. Thank you for this, huge but important feature done.
closes #547, closes #252