-
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
Discuss the value of lift
in the 6.0 world
#2911
Comments
What's end-user-consumer side changes once lift is gone? ergonomics around dot operator chaining will have changes? Also I recall there was some performance reason to choose lift for first time - with all higher order, what'll be differences around those as well? |
There would be no ergonomics changes around dot-chaining for the 90% use-case. For other use cases, it would mean that you couldn't build a custom Observable and have it compose through standard operators. So |
I am probably missing something, but what was the benefit of |
@felixfbecker it controls the creation of the Subscriber, which is what you really end up hooking into to get visibility into any operator. |
@benlesh Regarding your point 1, to what extent was using It's interesting to note that prior to its 5.0.0-rc, AngularFire2 used classes that extended In its 5.0.0-rc, AngularFire2 has been significantly refactored and no longer (as far as I can see) includes such classes. I'm curious as to whether such classes not playing super nice with TypeScript was a factor. Perhaps we could ask @davideast? |
I like @benlesh Why do pipeable operators eliminate benefit number 1? As far as I see, this use case would still work with (A) it's built as a composition of other common operators
|
@staltz can't any operator that can be implemented with (B) be imlpemented with export const myOperator = () => (source: Observable<T>) => new Observable<R>(observer => {
// operator code
}) ? |
@cartant We've documented the approach, however given how I've seen some pretty clever people struggle a little with that approach, I sincerely doubt it's wide spread. Also, given that not all operators are implemented with lift, I think this approach is still going to result in confusion. The pipeable operators mean we have none of these problems anymore.
@felixfbecker is talking about what I mean, @staltz... if all operators are just functions that return |
Oh, so maybe I misunderstood benefit 1, if it's all about "users could create custom operators on a class that extends Observable". I think I was talking the whole time about benefit 2, because I don't see benefit 1 as being that important.
Which ways?
Which operators? And is it out of impossibility or just oversight? |
Also agreed on getting rid of it. This along with #3057 should make the library simpler to contribute to, make the generated code smaller, and improve treeshaking. |
The Operator classes were an optimization tailored for what JIT'd well in v8 at the time (more details in the original Lifted Observable proposal, and below). If operator functions JIT (or tree-shake, etc.) better now, then we should do that. In fact, the interface Operator<T, R> {
call(subscriber: Subscriber<R>, source: any): TeardownLogic;
}
// these are both valid:
class OperatorA() {
call(sink, source) { return source.subscribe(sink) }
}
source.lift(new OperatorA());
function OperatorB(source) {
return source.subscribe(this);
}
source.lift(OperatorB); I've even proposed updating the Operator interface to reflect this (at the time the interface was written, TS didn't support hybrid types), but got push back that it could be confusing. What's the deal with
|
The one thing that I always liked about My understanding (however limited I will admit) is that |
@david-driscoll theoretically we can monkey-patch Observable.prototype.pipe = function customPipe(...operators) {
return operators.reduce((source, operator) => {
return operator(new Observable((sink) => {
return source.subscribe({
next(x) { console.log('captured:', x); sink.next(x); }
error: sink.error.bind(sink),
complete: sink.complete.bind(sink),
});
});
}, this);
} edit: I linked this in my answer above, but we also use |
Thanks for commenting on this @trxcllnt, I remember almost all of this from our original work, given our concerns with size, I'm trying to look at every avenue I can to reduce the footprint of the library. That's the major catalyst here. Another concern I have is that at some point, a standardized Observable will land, and it will not include a As of even a week ago, playing around with this, I'm pretty sure I want to keep What do you think about moving off as a standalone function?
|
I think lift is staying as is for now |
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. |
I'd like to talk about removing
lift
and the ceremony around that.Why we had
lift
lift
is a really cool architecture for a couple of reasons:lift
to return the custom class, allowing the custom operators to mix in the same dot-chains as standard operators.Lift In The World Of Pipeable Operators
With Pipeable operators, we're no longer dot-chaining everything. This means that controlling the return type no longer matters, and developing a custom operator is as simple as writing a higher-order function. This calls into question the value of
lift
, since that eliminates benefit number 1 listed above.Given that all operators will likely go through the
pipe
method, or otherwise carry a similar signature. It seems like there would be ample ways to hook our operators for any sort of tooling we might want to do. Either way, the promise oflift
as a centralized point to hook everything never came through, and in fact there were always some operators that just didn't uselift
so it wouldn't have been perfect.Proposed change for 6.0
Remove lift process entirely.
I think the benefit here will be a sharp reduction in the size of the library. As there will no longer be a need for all of the
Operator
classes in the library, nor will there be calls tolift
or implementations oflift
onObservable
orSubject
.Interested in all of your thoughts... especially @trxcllnt
The text was updated successfully, but these errors were encountered: