-
Notifications
You must be signed in to change notification settings - Fork 84
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
[Fix] websocket connecting and patchDB connection monitoring #1738
Conversation
375ee14
to
ad037b1
Compare
frontend/projects/ui/src/app/app/global/services/patch-monitor.service.ts
Outdated
Show resolved
Hide resolved
map(([connectionFailure, progress]) => { | ||
if (connectionFailure === ConnectionFailure.None) { | ||
return null | ||
} else if (!!progress && progress.downloaded === progress.size) { |
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.
No need for else
since there are return
statements.
@@ -31,7 +32,7 @@ export class MarketplaceListPage { | |||
.pipe( | |||
filter(data => exists(data) && !isEmptyObject(data)), | |||
first(), | |||
switchMapTo(this.marketplaceService.getPackages()), | |||
switchMap(() => this.marketplaceService.getPackages()), |
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.
This could be the reason why now it requests packages upon reconnection — as opposed to switchMapTo
now getPackages
is getting called everytime stream gets here.
frontend/projects/ui/src/app/services/patch-db/patch-db.factory.ts
Outdated
Show resolved
Hide resolved
.pipe( | ||
debounceTime(420), | ||
tap(cache => { | ||
this.bootstrapper.update(cache) |
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.
The subscription is now here only to update the bootstrapper?
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.
Correct
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 wonder if it makes sense to pass it to patch-db
to be added here:
public cache$ = this.source$.pipe(
tap(res => this.store.update(res)),
map(_ => this.store.cache),
tap(cache => this.bootstrapper.update(cache),
shareReplay(1),
)
Because we're not really starting anything here...
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'm game, will require some thought as to keeping PatchDB generic with regard to initial cache and bootstrapper updating.
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.
Looks good to me, added a few cosmetic comments.
@@ -1,4 +1,4 @@ | |||
import { Component } from '@angular/core' | |||
import { ChangeDetectionStrategy, Component } from '@angular/core' |
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.
Look like you forgot to use it
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.
No, I tested it and doesn't work on this page. Will remove the import
// check for updates to EOS and services | ||
this.checkForUpdates(ui) | ||
// show eos welcome message | ||
this.showEosWelcome(ui['ack-welcome']) |
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.
Seems like our next stop, after this is merged, is to switch this to a declarative modal.
.pipe( | ||
debounceTime(420), | ||
tap(cache => { | ||
this.bootstrapper.update(cache) |
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 wonder if it makes sense to pass it to patch-db
to be added here:
public cache$ = this.source$.pipe(
tap(res => this.store.update(res)),
map(_ => this.store.cache),
tap(cache => this.bootstrapper.update(cache),
shareReplay(1),
)
Because we're not really starting anything here...
No description provided.