-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(rx): A new method of piping has been added #7229
Conversation
+ Adds `r` a new method of piping observables. This allows any valid observable input to be passed as the first argument, and it will implicitly convert the argument to an Observable and pass it to the function that may be present in the second argument, the return value of THAT function is then passed an argument to the next function, and so on + Corrects the types of `Observable#pipe` such that anything can be returned at any step of the functional chain, however the first pipeable function must accept an `Observable<T>`. + Adds dtslint and runtime tests for `r` and `pipe`. NOTE: This does NOT deprecate`Observable#pipe`. That will be done in a follow up, and we need to define what timeline we'll take to remove that, as it's an API that is broadly used - could be v9, could be v10, could be never. At this point, this is alpha/beta functionality. BREAKING CHANGE: `Observable#pipe` now allows any pipeable unary function as an argument, just so long as the types properly compose, this means in some cases it will now return `unknown` instead of `Observable<unknown>` the workaround is just to cast the result for those cases. (or you can break your operator piping into smaller pipe sets) Related ReactiveX#7203
After some Core Team discussion, and some testing of the syntax, we've decided to move the implementation to where it takes an array of operators as the second argument. One of the example cases that compelled this change was this: r(
combineLatest([a, b]),
map(([a, b]) => a + b),
filter(n => n > 10),
)
.subscribe(console.log) In the above scenario, muscle-memory for RxJS users leads them to believe that The following was deemed to be more readable: r(
combineLatest([a, b]),
[
map(([a, b]) => a + b),
filter(n => n > 10),
],
)
.subscribe(console.log) This PR is currently brokenI knowingly pushed it up this way so it can be addressed later. There's a problem with the TypeScript types when we do this. TypeScript can't seem to figure out how to type the operators as they go through the array. Actions
|
cc @DanielRosenwasser 🙏❤️ |
Personally I am not a huge fan of the new array notation. One of my main issues when dealing with RxJS operators is getting the damn brackets right when refactoring code. Introducing new ones won't help. |
After speaking offline with the TypeScript folks, there's just no way to properly type the array notation. So this is going back to the more "flat" approach. |
I know we talked about this @kwonoj ... but I'm really not sure I love |
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.
LGTM, just a few questions about any
usage and a nitpick.
op6: UnaryFunction<E, F>, | ||
op7: UnaryFunction<F, G>, | ||
op8: UnaryFunction<G, H>, | ||
op9: UnaryFunction<H, Observable<I>>, | ||
...operations: OperatorFunction<any, any>[] |
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.
Can we replace the any
usage here with unknown
?
op7: UnaryFunction<F, G>, | ||
op8: UnaryFunction<G, H>, | ||
op9: UnaryFunction<H, I>, | ||
...operations: UnaryFunction<any, any>[] |
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.
Can we replace the any
usage here with unknown
?
@@ -344,7 +350,7 @@ export class Observable<T> implements Subscribable<T> { | |||
* .subscribe(x => console.log(x)); | |||
* ``` | |||
*/ | |||
pipe(...operations: OperatorFunction<any, any>[]): Observable<any> { | |||
pipe(...operations: UnaryFunction<any, any>[]): unknown { |
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.
Can we replace the any
usage here with unknown
?
src/internal/util/rx.ts
Outdated
fn7: UnaryFunction<F, G>, | ||
fn8: UnaryFunction<G, H>, | ||
fn9: UnaryFunction<H, I>, | ||
...fns: UnaryFunction<any, any>[] |
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.
Same question as with pipe
: Can we replace the any
usage here with unknown
?
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.
🚀🚀🚀
...operations: OperatorFunction<any, any>[] | ||
): Observable<unknown>; | ||
pipe<A, B, C, D, E, F, G, H, I>( |
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 really wish we either had a dedicated pipe operator in js or at least had some way to use recursive/conditional types powerful enough to support not needing to do this.
src/internal/util/rx.ts
Outdated
* ```ts | ||
* r(source, map(x => x + 1), filter(x => x % 2 === 0)); | ||
* pipe(from(source), map(x => x + 1), filter(x => x % 2 === 0)); | ||
* pipe(source, from, map(x => x + 1), filter(x => x % 2 === 0)); |
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.
Probably also worth calling out that rx([1, 2, 3]);
is an Observable<number>
and not Observable<number[]>
in the documentation (it's already in the type tests)
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.
A lot of minor issues found: r
to rx
rename should be updated in couple of places, some generics issues need to be addressed and docs needs more info and some updates.
src/internal/util/rx.ts
Outdated
import { ObservableInput, UnaryFunction } from '../types'; | ||
import { pipeFromArray } from './pipe'; | ||
|
||
export function rx<T, A>(source: ObservableInput<A>): Observable<A>; |
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 T
generics are unused in the entire file. Please remove them.
src/internal/util/rx.ts
Outdated
* pipe(source, from, map(x => x + 1), filter(x => x % 2 === 0)); | ||
* ``` | ||
* | ||
* Furthermore, `r` can be used to create an observable and pipe it in any number of ways. For example: |
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.
* Furthermore, `r` can be used to create an observable and pipe it in any number of ways. For example: | |
* Furthermore, `rx` can be used to create an observable and pipe it in any number of ways. For example: |
src/internal/util/rx.ts
Outdated
export function rx<T, A>(source: ObservableInput<A>): Observable<A>; | ||
export function rx<T, A, B>(source: ObservableInput<A>, fn2: UnaryFunction<Observable<A>, B>): B; | ||
export function rx<T, A, B, C>(source: ObservableInput<A>, fn2: UnaryFunction<Observable<A>, B>, fn3: UnaryFunction<B, C>): C; |
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
T
generics are unused in the entire file. Please remove them.
Or use them ☝️ like this:
export function rx<T, A>(source: ObservableInput<A>): Observable<A>; | |
export function rx<T, A, B>(source: ObservableInput<A>, fn2: UnaryFunction<Observable<A>, B>): B; | |
export function rx<T, A, B, C>(source: ObservableInput<A>, fn2: UnaryFunction<Observable<A>, B>, fn3: UnaryFunction<B, C>): C; | |
export function rx<T>(source: ObservableInput<T>): Observable<T>; | |
export function rx<T, A>(source: ObservableInput<T>, fn1: UnaryFunction<Observable<T>, A>): A; | |
export function rx<T, A, B>(source: ObservableInput<T>, fn1: UnaryFunction<Observable<T>, A>, fn2: UnaryFunction<A, B>): B; |
...and so on.
FWIW: starting with T
and fn1
(instead of fn2
) is much better IMO as users might wonder where fn1
is when reviewing the docs.
This is the docs as I suggested here:
vs how it is now:
* | ||
* Furthermore, `r` can be used to create an observable and pipe it in any number of ways. For example: | ||
* | ||
* ```ts |
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.
In order for these examples to work on StackBlitz, I'd like to introduce imports:
* ```ts | |
* ```ts | |
* import { rx, of } from 'rxjs'; |
* }, | ||
* }); | ||
* ```` | ||
*/ |
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.
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 fixed the types up, which should resolve that a little.
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 also added @param
and @return
docs.
src/internal/util/rx.ts
Outdated
* | ||
* ```ts | ||
* r(source, map(x => x + 1), filter(x => x % 2 === 0)); | ||
* pipe(from(source), map(x => x + 1), filter(x => x % 2 === 0)); |
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 doesn't seem to be valid. This does:
* pipe(from(source), map(x => x + 1), filter(x => x % 2 === 0)); | |
* pipe(() => from(source), map(x => x + 1), filter(x => x % 2 === 0)); |
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.
Fixed. It was a little different than what that said.
* | ||
* The following are equivalent: | ||
* | ||
* ```ts |
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.
Adding the source would help a lot. E.g.
const source = Promise.resolve(1);
or
const source = [1, 2, 3];
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 added more context.
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Nicholas Jamieson <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
Co-authored-by: Mladen Jakovljević <[email protected]>
+ Adds some more tests around `Observable#pipe`, ensuring arbitrary unary functions work + Adds more dtslint tests for `rx`. + Fixes some comments + Improves types and runtime for args in `rx` function
import { pipeFromArray } from './pipe'; | ||
|
||
export function rx<A>(source: ObservableInput<A>): Observable<A>; | ||
export function rx<A, B>(source: ObservableInput<A>, fn2: UnaryFunction<Observable<A>, B>): B; |
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 still in favor of using fn1
instead of fn2
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.
LGTM overall, there are just some nitpicks that I'd like to be considered fixing.
rx
a new method of piping observables. This allows any valid observable input to be passed as the first argument, and it will implicitly convert the argument to an Observable and pass it to the function that may be present in the second argument, the return value of THAT function is then passed an argument to the next function, and so onObservable#pipe
such that anything can be returned at any step of the functional chain, however the first pipeable function must accept anObservable<T>
.rx
andpipe
.NOTE: This does NOT deprecate
Observable#pipe
. That will be done in a follow up, and we need to define what timeline we'll take to remove that, as it's an API that is broadly used - could be v9, could be v10, could be never. At this point, this is alpha/beta functionality.BREAKING CHANGE:
Observable#pipe
now allows any pipeable unary function as an argument, just so long as the types properly compose, this means in some cases it will now returnunknown
instead ofObservable<unknown>
the workaround is just to cast the result for those cases. (or you can break your operator piping into smaller pipe sets)Addresses #7203
Note for later: Make sure to update squashed commit message to reflect
rx
and notr
.