Upgrade rxbeach to rxjs7 #478
Conversation
| // NOTE: .subscribe(this.subject) does not work | ||
| // All tests pass with it, but in practice there are cases | ||
| // where the subject isn't updated as expected | ||
| // .subscribe({ | ||
| // complete: () => this.subject.complete(), | ||
| // error: (err) => this.subject.error(err), | ||
| // next: (state) => this.subject.next(state), | ||
| // }); |
There was a problem hiding this comment.
Seems this can be removed, unless QA finds anything
| reducerFn: Reducer<State, any> | ||
| ) => { | ||
| const wrapper = (state: State, payload: any, namespace?: string) => | ||
| reducerFn(state, payload, namespace); | ||
| if (trigger instanceof Observable) { | ||
| if (!Array.isArray(trigger) && isObservableInput(trigger)) { |
There was a problem hiding this comment.
This doesn't really sit well with me, but I think we can fix it after this release
There was a problem hiding this comment.
As we discussed, I think from an interface and readability point of view it would be better to have a actionReducer and stateReducer, that would probably also allow to make the code here a bit simpler.
|
@tlaundal Should we change it to ready to review? |
Not yet. I'm making some small changes and plan to rewrite the history. We need to rewrite the history to get a proper changelog when we run the next release. |
Updates to RxJS version 7.x. This RxJS update contains many breaking changes, which leads to breaking changes in RxBeach as well. These changes also required us to reimplement `PersistentReducedStateStream`, it is now called `ObservableState`. Co-authored-by: Tobias Laundal <tobias.laundal@ardoq.com> BREAKING CHANGE: See breaking changes from RxJS 7. BREAKING CHANGE: `persistentStateStream` no longer returns an instance of a subclass of `Observable`, but rather the new class `ObservableState`.
tlaundal
force-pushed
the
upgrade-rxbeach-to-rxjs7
branch
from
ed5125d
to
6fc0298
Compare
tlaundal
requested review from
davidnegley,
nikican,
rafaa2,
andreajl and
chriskr
| get destroyed(): boolean { | ||
| return this.subject.closed; | ||
| } | ||
| } |
There was a problem hiding this comment.
Looks nice! Quite a bit simpler than the old version.
| reducerFn: Reducer<State, any> | ||
| ) => { | ||
| const wrapper = (state: State, payload: any, namespace?: string) => | ||
| reducerFn(state, payload, namespace); | ||
| if (trigger instanceof Observable) { | ||
| if (!Array.isArray(trigger) && isObservableInput(trigger)) { |
There was a problem hiding this comment.
As we discussed, I think from an interface and readability point of view it would be better to have a actionReducer and stateReducer, that would probably also allow to make the code here a bit simpler.
tlaundal
force-pushed
the
upgrade-rxbeach-to-rxjs7
branch
from
b755b75
to
6fc0298
Compare
Upgrade all dependencies to latest version, and in particular update RxJS to 7.5.6.
RxJS has a bunch of breaking changes, the consequences of these in RxBeach are:
mergeandcombineLatestexported from the operators namespace have been renamed with thewithsuffixPersistentReducedStateStreamhas been deleted, as it relied on extendingObservable, and would probably not work in RxJS 8persistentReducedStreamnow returns instances of the new classObservableState, which does not extendObservable. Some types will need to be changed.ObservableStateimplementsInteropObservableit integrates nicely into RxJS, but some custom tooling might need to be updated to acceptObservableInputas arguments, and use thefromfunction from RxJSThis PR is read for review, and possibly merge. It is in use in a team environment in ardoq-front for testing by QA. Once this PR passes review, we need to: