-
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
RFC: A new way of "piping" #7203
Comments
I like the approach! |
💯 |
Seems great! Is "rx" the final proposed name? I know it'd be a breaking change, but what if... import { pipe, flow } from "rxjs";
// Pipes a value into operators
const derived$ = pipe(source$, ...operators)
// Or any other name really. This is what the standalone "pipe" is at the moment.
const customOperator = flow(...operators) Would it be too bad? 😬 Mentally, |
|
|
|
Not sure how you'd use rx without a pipe concept to be able to tree shake it, but I think this is a great way to solve interop; I like the ergonomics. |
I like it. Ship it |
I know people would probably hate it, but have you considered a function that returns a function that would pipe the operator arguments and call the result with the observable passed. rx(source$)(...operators) |
One option is to simply make a function with 2 arguments (the second is an array): rx(fromEvent(document.body, "mousemove"), [
debounceTime(2),
map(({ clientX }) => clientX),
take(2),
takeUntil(destroy$),
]); |
@BigAB why do you like this shape? Is there a use case? |
@timdp Because it's not really just a Technically it's |
Personally, I don't think that makes a huge difference, but I do see your point. But if How about |
const step2$ = rx(step1$, map(n => n + n))
const step3$ = rx(step2$, filter(n => n > 0))
step3$.subscribe(console.log) would the above be the way to "wrap" an observable from the outside?
import { pipe } from "rxjs";
import { tap } from "rxjs/operators";
function logWithTag(tag: string) {
return pipe(
tap(v => console.log(`logWithTag(${tag}): ${v}`)),
// put whatever operators you need here
);
}
// and then
source$.pipe(logWithTag('my-tag')).subscribe(whatever) with function withLog(source: ObservableInput<A>, tag: string) {
return rx(source,
tap(v => console.log(`logWithTag(${tag}): ${v}`)),
// put whatever operators you need here
);
} which is doable, but moves away from classical functional composition, I'm afraid. What's your take on this? |
I don't really like the syntax since it wouldn't be compatible with the pipeline operator (which would be a big benefit for RxJS) right? Here is an example (in which you are even mentionned haha) |
@LcsGa currently the TC39 pipeline operator is not the one as in the example. The operators in RxJS would be usable in the F# pipe operator proposal, where every operator is a function that takes one argument and returns one value, which is passed to the next one. TC39 went with the hack syntax, where you need to explicitely pass in a "placeholder" which says where the value is passed onto. So with TC39 pipes and current operators, it would look like: interval(100)
|> take(5)(%)
|> map(v => v + 3)(%) More about this discussion here #7194 |
@voliva Thanks for the information, I wasn't aware of this! |
I like a clear separation between the subject of the call and the composition of functions. Using an array as the second arg is pretty good and achieves the separation I want, I just sort of "didn't like it" 🤷, but it's not bad : const obs$ = rx(source$, [
debounceTime(2),
map(({ clientX }) => clientX),
take(2),
takeUntil(destroy$),
])
With it first it sort of looks like the F# pipe operator shape... const obs$ = rx(source$)(
debounceTime(2),
map(({ clientX }) => clientX),
take(2),
takeUntil(destroy$),
) I actually really like the idea of calling it
EDIT: If it was export const pipe = (...fns) => (input) => fns.reduce((r, fn) => fn(r), input);
export const pipeThrough = (source) => (...operators) => pipe(...operators)(source); ...you could use it on non-observables too 🤷 (and just check for const result = pipeThrough(3)(
(n) => n*2,
(n) => n + 1,
(x) => `WHOA ${x}!`
)
// result === "Whoa 7!" |
I don't know if I missed the "Bonus" section the first time I read it but, after reading it, yeah getting all the rx(source$, [
debounceTime(2),
map(({ clientX }) => clientX),
take(2),
takeUntil(destroy$),
]) ...is the better choice, because you get the great "turn anything into an Rx observable" from |
CORE TEAM:
There's value here, we want to do this. |
@benlesh Can we use from(source$, [
debounceTime(2),
map(({ clientX }) => clientX),
take(2),
takeUntil(destroy$),
]) |
@demensky - do you think that's stretching intention of |
Extending |
Personally, I still like
I'm still open to other names though. I'd just prefer that it's short and to the point. Also something to think about, Using rx(mouseDowns$, [
exhaustMap((e) => {
const startX = e.clientX;
return rx(mouseMoves$, [
takeUntil(mouseups$),
map((e) => e.clientX - startX),
]);
}),
]).subscribe(handleDrag); Using rx(
mouseDowns$,
exhaustMap((e) => {
const startX = e.clientX;
return rx(
mouseMoves$,
takeUntil(mouseups$),
map((e) => e.clientX - startX)
);
})
).subscribe(handleDrag); Honestly, and I think this is weird, the |
I would also expect (Personally, I'm still not sold on the |
Let's call it Kidding… Do I understand correctly that this For example function map<T, R>(source$: ObservableInput<T>, project: (value: T, index: number) => R): Observable<R>; It turns out that |
CORE TEAM: How about The concern with |
+ 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
+ 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
Personally I would be in favor of |
Here is the thing |
I like this approach, but what you say about this approach ? const obs$ = rx( source$, compose([
debounceTime(2),
map(({ clientX }) => clientX),
take(2),
takeUntil(destroy$)
])
) |
I came here to say that |
It's okay, but I find the array passed to const obs$ = rx( source$, compose(
takeUntil(destroy$),
take(2),
map(({ clientX }) => clientX),
debounceTime(2),
)) ...which is fine, I guess. But I still like this one better: rx(source$, [
debounceTime(2),
map(({ clientX }) => clientX),
take(2),
takeUntil(destroy$),
]) |
|
I personally didn't really like the idea of First, I would argue that Secondly, I really prefer the array signature signature over "rest" operator. I think code becomes more readable. Something to think about is if we want to allow developers to skip the array signature if it's only one operator? const obs$ = rx(source$, [take(1)]);
// could have signature
function rx<A, B>(source: ObservableInput<A>, a: OperatorFunction<A, B>): Observable<B>;
function rx<T>(source: ObservableInput<T>, operators: Array<OperatorFunction<T, any>>): Observable<unknown>;
// and allow for both
const obs1$ = rx(source$, take(1));
const obs2$ = rx(source$, [take(1), map(x => x * 2)]); Thirdly, I'm curious about the conversion to promises. Consider this existing code (using DI from Angular): async function() {
await doSomething();
const fullname = await firstValueFrom(this.auth.user$.pipe(
take(1),
map(({ firstname, lastname }) => `${firstname} ${lastname}`)
));
} With the new approach, it would be nice if this wasn't so clunky. Perhaps a new operator as well to convert to This would end up looking like this with async function() {
await doSomething();
const fullname = await firstValueFrom(rx(this.auth.user$, [
take(1),
map(({ firstname, lastname }) => `${firstname} ${lastname}`)
]));
} But perhaps we could have something like async function() {
await doSomething();
const fullname = await rx(this.auth.user$, [
take(1),
map(({ firstname, lastname }) => `${firstname} ${lastname}`),
firstValueToPromise() // or lastValueToPromise()
]);
} I see this pattern a lot where you want to ensure execution order and using a |
Cool. rx(
input$,
debounceTime(500),
subscribeUntilUnmount((v) => emit("value-change", v))
); |
CORE TEAM: |
* feat(r): A new method of piping has been added + 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 #7203 * refactor: rename to `rx` * docs: fix `r` to `rx` reference. Co-authored-by: Mladen Jakovljević <[email protected]> * chore: fix description Co-authored-by: Nicholas Jamieson <[email protected]> * chore: fix return type of part of signature. Co-authored-by: Mladen Jakovljević <[email protected]> * chore: correct docs to say `rx` and not `r`. Co-authored-by: Mladen Jakovljević <[email protected]> * chore: address comments + 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 --------- Co-authored-by: Mladen Jakovljević <[email protected]> Co-authored-by: Nicholas Jamieson <[email protected]>
the basic of, from and fromEvent should not be changed, people are used to use them and rx for an operator is just a bad name! |
It can seem like inventing the wheel again, because we still need pipes and from operator but on the other hand the from operator is nothing else but Observable. The difference is that from is more efficient for developers than writing an observable each time. The question is how efficient for developers will be this new approach ? Migration from pipes towards rx method can be challenging for big projects and can create an additional layer that people would mix. Nevertheless the concept is fairly cool. |
Given the move away from lift: #7201 #7202
We can make our observable even more compatible with other observables by migrating us away from the
pipe
method. This is not something that would happen immediately, rather over a very, very long period. The idea is to increase compatibility and improve ergonomics.Problem: It's annoying to have to
from(x)
types in order to use our operators with themThere's a common issue where people want to convert other things to observables in order to use our operators. Whether it's an array or a promise, there are times were you need to do
from
and the only reason you're doing it is because you want that.pipe
method, and you want an observable to operate on.Problem:
pipe
is a methodThe second one is more of a problem, honestly. If people implement code with the expectation that
pipe
exists as a method on whatever observable they get, then even while we've made sure our operators will work with any "same-shaped" observable, that custom code will break, because it's expectingpipe
to be a method.Proposal
A new
rx
method that accepts anObservableInput
as the first argument, and a rest of operators:(Technically,
rx
is just a standard functional pipe with some specific types, it could be the samepipe
method we expose under the hood)The usage would be as such:
Bonus?
from
could migrate to just berx
as wellFor a long time people have complained about
from
being a horrible name for an exported function. (Although I do likeimport { from } from './from'
in our codebase 😄). This would mean that couple could technically just do:rx(somePromise)
and it's the same asfrom(somePromise)
.The text was updated successfully, but these errors were encountered: