-
Notifications
You must be signed in to change notification settings - Fork 3k
STRAWMAN: pipe method #2489
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
STRAWMAN: pipe method #2489
Conversation
Allows composition of pipeable operator functions to avoid mutating Observable prototype
Now we can assert that two observables that originated from TestScheduler.prototype.createColdObservable or TestScheduler.prototype.createHotObservable are equivalent with `expectObservable(cold('--a--b--c--|')).toBe(cold('--a--b--c--|'))` This feature was mostly added to test the new pipe operators
Another con worth noting in comparison to the |
I like it pipe, no op(), would have been nice to call it more what it does like applyAll, pipe is probably an established name but it doesn't hurt to think wether ppl will have to look at the manual or intuitively understand just based on the name. Apply is a dangerous name in javascript land though :) |
Two more random pitches: Operator patching, but they must use a symbol to access the property.If this is used, the operator is added to the prototype under a symbol, so anyone else using it doesn't need to know if it's been imported before or not, but they cannot accidentally depend on the operator existing without importing it themselves! I'm initially pretty drawn to this solution until (if...) we get some sort of pipe/bind operator in JS. Though indeed this is not yet typesafe in TS import { map } from 'rxjs/symbols/operator/map';
Observable.of(1, 2, 3)
[map](x: number) => x + x)
[map](x: number) => x * x)
[map](x: number) => x + '!!!')
.subscribe(x => console.log(x)); Weird chaining thingI don't know what to call this, but I believe someone else suggested it in earlier discussions. The main con to this is that the average JS user will probably be like wtf how does this work, even though it's really fairly simple chaining. We could name it Observable.prototype.sorcery = function () {
const chain = (operator, ...args) => operator.apply(this, args).sorcery();
chain.subscribe = (...args) => this.subscribe(...args);
return chain;
};
Observable.of(1, 2, 3)
.sorcery()
(map, (x: number) => x + x)
(map, (x: number) => x * x)
(map, (x: number) => x + '!!!')
.subscribe(x => console.log(x)); |
The symbol one is interesting, but in IE they'd end up with the same problem (depending on the Symbol polyfill). I like it because it was one of the first things I thought of. "Hey I want to alter the prototype without trampling other people's business" |
@benlesh hmm I don't follow about IE. Even if the polyfill just used a unique string, they would have to know that unique string and use it directly without importing, which people wouldn't do? Observable.of(1, 2, 3)
['i-am-a-fake-symbol-for-map'](x: number) => x + x)
['i-am-a-fake-symbol-for-map'](x: number) => x * x)
['i-am-a-fake-symbol-for-map'](x: number) => x + '!!!')
.subscribe(x => console.log(x)); |
@jayphelps some folks replace Symbol with |
@benlesh Yeah, I can imagine. import 'bad-symbol-polyfill';
// I don't think people will ever do this, at least without knowing they're
// being hacky and that's on them
Observable.of(1, 2, 3)
['@@map'](x: number) => x + x)
['@@map'](x: number) => x * x)
['@@map'](x: number) => x + '!!!')
.subscribe(x => console.log(x)); I think the primary concern should be to prevent |
Really having a hard time understanding why we can't just use
Seems like |
For reference, this is what we do in xstream (xstream
If the closure in the implementation is an issue for perf, we can also return a function or an Operator class with a PS: and TypeScript stays happy. |
@staltz I agree. |
Also, I must say, regarding his implementation of export function map<T, R>(
project: (value: T, index: number) => R,
thisArg?: any
): (source: Observable<T>) => Observable<R> {
return (source: Observable<T>) => mapProto.call(source, project);
} This seems to imply a too-high-order function. Does this function not ignore
|
Some of the typings seem a bit verbose... export function map<T, R>(
project: (value: T, index: number) => R,
thisArg?: any
) {
return (source: Observable<T>) => mapProto.call(source, project);
}
// Which resolves to something that behaves like...
map(x => !!x)(source);
// so then pipe will see a set of types like...
let pipes: ((source: Observable<T>) => Observable<T>)[] |
Overall I'm not a huge fan of the proposal simply because it makes typings manual in all situations (I tried again and couldn't make it work sadly 😞) However if this is the way we want to go, here is a playground that allows us to flow the types through a little better than what exists in this PR. Basically it boils down an extended interface for interface IPipeOperator<T> {
<R1>(op1: IPipeMethod<T, R1>): Observable<R1>;
<R1, R2>(op1: IPipeMethod<T, R1>, op2: IPipeMethod<R1, R2>): Observable<R2>;
<R1, R2, R3>(op1: IPipeMethod<T, R1>, op2: IPipeMethod<R1, R2>, op3: IPipeMethod<R2, R3>): Observable<R3>;
<R>(...fns: ((x: Observable<T>) => Observable<any>)[]): Observable<R>;
} I would recommend we expand this one out to about 9 overloads, to allow for a reasonably large amount of piped operators. Which resolves down to, something like the following. Basically the types must all be on each operator (which sucks) but then if the types are wrong as they flow through, then you will get compiler errors at the very least. let o: Observable<string>;
let o2: Observable<number> = o.pipe(
map<string, string>(x => x),
map<string, number>(x => x.length * 2) // map<number, number> would be a compiler error
); Here are some related issues about the matter, it looks like something that may be solved, but I doubt it will be solved anytime soon. microsoft/TypeScript#9366 |
Looking further at it, I agree with @staltz that this is essentially something like... pipe(this: Observable<T>, ...fns: ILetMethod[]): Observable<T> {
const composed = compose.apply(this, fns);
return composed(this);
} |
We discussed AFAICT three main reasons were brought up:
source
.let(s => filter.call(s, x => x > 1))
.let(s => map.call(s, x => x + 10))
// vs.
source
.pipe(
filter(x => x > 1),
map(x => x + 10)
)
// vs.
source
.op(filter, x => x > 1)
.op(map, x => x + 10)
// etc... I think we need some clarity on the goals of these various proposals. e.g. are we trying to appease library authors themselves who don't want to bleed implementation details aka leak operators or are we trying to create a new paradigm that we'll pitch as the preferred alternative of operator patching? Initially the discussion in #2034 seemed to be the former, but I've seen conversations between Ben and Igor suggesting to me the later is also now a possible goal. If this is just for library authors, I think type safety is certainly preferred but is less critical than if this is intended for general usage by app devs. I mostly bring this up cause we otherwise might continue being deadlocked on this topic. |
Also interestingly, Roughly... function op<R>(func: (...args:any[]) => ((source: Observable<T>) => Observable<R>), ...args: any[]): Observable<R> {
return func(...args)(this);
} If source
.let(filter(x => x > 1))
.let(map(x => x + 10)) or we refactor source
.let(
filter(x => x > 1),
map(x => x + 10)
); |
@benlesh why aren't we just using a form of transducers if we're going to a point of operator fusion instead of just some shortcutting? |
@david-driscoll hmmm from what I can see it isn't the same. Ben's proposed Edit: it seems you're referring to a new, proposed change/extension to |
@staltz is right. |
Those are fine for synchronous actions... I played with this via a "Scannable" once upon a time. The goal here is to give people easy access to using all of the operators without mutating prototype. This is an ask from the Angular team, as well as others I've talked to that work on libraries that are to be consumed. Falcor-router had issues with this as well. (The issue being people depending on operators they didn't add themselves, and then getting broken when a dependency stops adding the operators) |
To add to @benlesh description of why something is needed, I feel this is also a major problem for the general user as well when they are testing. If you use operator patching in your tests, your app may be accidentally depending on that operator but not importing itself. All tests pass but your app fails on its own. |
@alancnet neither provides type safety. |
@jayphelps is that a simple fix? |
@alancnet for For
|
@alancnet also, I don't think the ego quip is productive. I don't believe Ben is having ego issues. The problem is complex and has been in discussion for almost a year in many forms, including discussing the overlap with Edit: rereading my response, it may come off as a little harsh. Sorry about that. I mostly want to keep the discussion away from personal stuff--I know I would feel upset if someone publicly implied my ego was getting in the way of a productive conversation. He may or may not have. Of course, if you do feel someone is having ego issues (we all do from time to time) you might bring it up to them in private. I would bet most of the time it's just a misunderstanding from lack of context combined with the cold nature of comments in text. Or at least they won't feel publicly attacked and things escalate. |
We could just do what @david-driscoll proposed
Yes it would require some new code for these operator factories, but all these 3 problems would be solved:
Or is the concern here that an operator factory is slightly slower? Wouldn't be nice for rxjs users to be confused around
Just a playful thought: RxJS could come with zero operators, each operator would live in its own package, then we would have presets. Babel style. And |
@staltz If changing
I just took a deeper look and can confirm, it doesn't seem possible to make it type safe. Someone who knows more TypeScript tricks might know of some way though. I don't have any concerns about initial performance of applying operators as long as the perf during data flow isn't affected. |
No, |
We can use the following interface to help enforce "type flow" (in that if the specified types are incorrect, it will be a compiler error). The higher order methods however will have to be strictly typed at creation. type ILetMethod<T, R> = (x: Observable<T>) => Observable<R>;
interface ILetOperator<T> {
<R1>(op1: IPipeMethod<T, R1>): Observable<R1>;
<R1, R2>(op1: IPipeMethod<T, R1>, op2: IPipeMethod<R1, R2>): Observable<R2>;
<R1, R2, R3>(op1: IPipeMethod<T, R1>, op2: IPipeMethod<R1, R2>, op3: IPipeMethod<R2, R3>): Observable<R3>;
<R>(...fns: ((x: Observable<T>) => Observable<any>)[]): Observable<R>;
} Giving an example of... let o: Observable<string>;
let o2: Observable<number> = o.let(
map<string, string>(x => x),
map<string, number>(x => x.length * 2) ,
map<string, number>(x => x * 10) // compiler error
); versus let o: Observable<string>;
let o2: Observable<number> = o.let(
map(x => x), // T: {}, R: {}
map(x => x.length * 2), // T: {}, R: {}
map(x => x.length * 10) // T: {}, R: {} // no compiler error
); |
Derp I left out what |
and replace |
Correct... I was more like "D'oh! Why didn't I notice that?". I really don't care if I have to throw out work. |
I agree with this. However, I don't see the harm in allowing |
So I did a little bit of thinking in the car ride home, and now have the idea of a set of operators that will work with let, or using Russian doll syntax. let o: Observable<string>;
// using let
let o2: Observable<number> = o.let(
oo => map(oo, x => x.length * 2),
oo => map(oo, x => x.length * 10)
);
// using Russian doll
let o2: Observable<number> = map(map(o, x => x.length * 2), x => x.length * 10)); Basically the operators are just the prototype based operator, but instead of using the this context, the argument is passed as the first argument. I like this a little better, as they interact with I have both scripted out in #2529 as a test the different ideas, with unit tests for |
FWIW: I'm heavily in favor of pushing this forward. I'd like to either change |
What are your thoughts on the approach in #2529? Basically we have these signatures:
With the state of FWIW compared to |
This is now a thing in 5.5 beta. |
your face is now a thing in 5.5 beta |
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Background
There is a consistent problem with users using patch operators (
rxjs/add/operator/map
for example) and decoratiing the prototype, then having a consumer of their code depend on that map operator being there, without ensure that it is. This, in particular affects projects like Angular and Falcor (and I'm sure others).After talking with @IgorMinar about ways to solve this, a solution was proposed that's not much different than the
op
PR found here #2034. It does, however, look cleaner with a few exceptions.Proposal
compose
utility function to Rx. This is used to compose a rest args of functions into a single function like so:pipe
method toObservable
. Thispipe
method will take a rest args of "pipeable operators", which are higher-order functions that return(source: Observable<T>) => Observable<R>
. This, internally, uses thecompose
function above.Rx.Pipe.map
andrxjs/pipe/map
function that is a higher order function that returns a "pipeable operator". (used in the example above)Using Pipe:
vs the
op
/operate
proposal:Pros
op
/operate
solution, which relies on comma separation.op
/operate
solution.Cons
pipe
to the next.Other Things
This PR adds the ability to compare two test observables via
expectObservable(obs1).toBe(obs2)
. This was done to make it easier to test pipe operators as they are added.Using
Rx.compose
in unison withObservable.prototype.pipe
is a fairly powerful combination, enabling a user to compose a chain of operations and reuse it much more dynamically than they might be able to otherwise.