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

Wrapper: Handle SetApp for updated apps #267

Merged
merged 14 commits into from Apr 13, 2019

Conversation

Projects
None yet
2 participants
@sohkai
Copy link
Member

sohkai commented Mar 22, 2019

kernel.SetApp() is emitted when a new app is installed or updated (for all instances of the app).

We don't need to detect old emissions of this event, as we already fetch the latest values of the app proxies on initialization. However, we do need to listen to new events, as an app could be updated at run time.

This adds a subscription that will emit a new list of apps on this.apps, with any updated apps having an updated: true flag that will allow clients to detect and handle (e.g. stop and rerun any WebWorkers, etc.).

This is a breaking change.


Note: I've chosen "updated" as the terminology instead of "upgraded" as technically you could also be downgrading an app when this event is emitted.

  • Test against local chain
  • Update client to detect and swap out updated apps
  • Should probably write a test that includes mocked event emissions on SetApp

sohkai added some commits Mar 21, 2019

@sohkai sohkai requested a review from 2color Mar 22, 2019

@sohkai sohkai force-pushed the handle-set-app branch from d625ee5 to d5f7992 Mar 22, 2019

// Should only see it twice
t.fail()
}
++eventCount

This comment has been minimized.

Copy link
@sohkai

sohkai Mar 22, 2019

Author Member

Not sure if there's a better way to write this type of test when multiple emissions are expected.

@luisivan luisivan referenced this pull request Mar 22, 2019

Closed

App Center (upgrades only) #3

0 of 1 task complete

sohkai added some commits Apr 9, 2019

identifier
})
}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 10, 2019

Author Member

Note that these changes to the identifiers means that this is a breaking change, as the identifier and app lists are not merged by default anymore.

I hadn't realized that the previous logic could create infinite loops until I actually encountered them:

  1. Init apps, get apps observable
  2. Client starts apps
  3. Apps emit on the identifier observable
  4. Due to the combineLatest() logic, another apps emission would occur
  5. If the client then restarts the app due to the apps emission (e.g. in the case of an upgrade), the app could re-emit its identifier
  6. Repeat from 4

Hence, I've taken the easy way out by splitting the two concepts from each other and pushing the work to clients.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 11, 2019

Author Member

Since it's a breaking change, I also took the liberty of renaming identifier to appIdentifier to make it more clear at a glance what this observable is for.

sohkai added some commits Apr 10, 2019

// Dedupe until apps change
distinctUntilChanged((oldProxies, newProxies) => {
if (oldProxies.length !== newProxies.length) {
return true

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor

Shouldn't this be false? The comparison function returns a bool of whether two items are the same (based on the example in https://rxjs-dev.firebaseapp.com/api/operators/distinctUntilChanged)

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor

It's badly documented, but based on the example the comparison functions is isEqual rather than isDistinct

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 11, 2019

Author Member

Good catch! Fixed in e6d3afc.

}
),
// Switch to resolved array of promises
switchMap(Promise.all)

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor

The switchMap here will call Promise.all on every new array of promises returned from the previous operator (withLatestFrom) and emit the resolved values of Promise.all. Why however do we use switchMap over mergeMap?

Can't we have a situation in which two SetApp events are emitted and we want the changes of both?

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 11, 2019

Author Member

Can't we have a situation in which two SetApp events are emitted and we want the changes of both?

Hmm, yes, this is true. I had thought that mergeMap() and switchMap() are synonymous when it comes to single-emission observables being returned from their operators, but that's not true when the first event is still waiting to get processed when the second event fires.

There are still cases where you'd only be interested in the latest event, e.g. how the appIdentifiers are set up, but I think all our uses of switchMap() should be switched to mergeMap() 👍.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 11, 2019

Author Member

See eaabdec, the decision to use switchMap() or mergeMap(), as I understand it, is if we care about the throttled values.

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 11, 2019

Author Member

So I figured this one out; the one we usually want with promises is actually concatMap() because we'd like to queue each promise's resolution behind each other (rather than firing at will or throttling).

Will do as part of #268.

sohkai added some commits Apr 11, 2019

await instance.initAppIdentifiers()
// assert
instance.identifiers.subscribe(value => {
t.deepEqual(value, {})

This comment has been minimized.

Copy link
@2color

2color Apr 11, 2019

Contributor

What's the reason that we emit an empty object initially? Is that due to the consumers rely on there always being some value (at times an empty object like here)

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 11, 2019

Author Member

No particular reason, but an empty one seems reasonable to begin with, API-wise, since the app identifier mapping should alway "exist" (but may not have any identifiers declared inside it).

@2color

2color approved these changes Apr 11, 2019

@sohkai sohkai merged commit 244190f into master Apr 13, 2019

4 checks passed

License Compliance All checks passed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

sohkai added a commit that referenced this pull request Apr 13, 2019

feat: @aragon/wrapper api cleanup (#279)
With the breaking change in #267, this is a good chance to consolidate the API a bit 😄.

Also adds a few tests to the observables requiring "callbacks" (e.g. `performTransactionPath()`).

sohkai added a commit to aragon/aragon that referenced this pull request Apr 14, 2019

feat: restart app worker when upgraded (#688)
Coupled with aragon/aragon.js#267, this allows the client to restart an upgraded app's worker in-place.

We reset the cache on the upgrade as well, as there may be incompatibilities in the reduced state between different worker versions (as well as paired with their frontends). Note that app frontends are handled mostly transparently as we re-load their URL into the iframe upon navigation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.