Skip to content
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

Hack Pipeline Mitigation #6582

Closed
benlesh opened this issue Aug 31, 2021 · 11 comments
Closed

Hack Pipeline Mitigation #6582

benlesh opened this issue Aug 31, 2021 · 11 comments
Labels
AGENDA ITEM Flagged for discussion at core team meetings

Comments

@benlesh
Copy link
Member

benlesh commented Aug 31, 2021

NOTE: This is NOT an intent to implement, nor is it an endorsement of the Hack pipeline. This is just a place to list plans if we're forced to support it. The F# style pipeline is much more ideal.

Given that the Hack Pipeline operator has been pushed to Stage 2, we need to prepare for the reality that we might be stuck with that operator. RxJS still wishes to move to whatever pipeline operator is available, and proceed using web/JavaScript standards wherever possible.

Hack Pipeline

Basically, it's a way of piping functions that involves a magic ^ token to declaratively decide where to pass/use the result of the left hand side:

const add = (a: number, b: number) => a + b;

const result = 1 |> add(1, %) |> Math.pow(4, %); // 16

The problems RxJS faces

Currently our "pipeable operators" rely on higher-order functions. Using these with the Hack Pipeline will be cumbersome, ugly and hard to read:

// Now:
source$.pipe(
  filter(x => x % 2 === 0),
  map(x => x + x),
  concatMap(x => of(x).pipe(delay(1000)))
)
.subscribe(console.log)

// Hack:
source$
  |> filter(x => x % 2 === 0)(^)
  |> map(x => x + x)(^)
  |> concatMap(x => of(x) |> delay(1000)(^))(^)
  |> %.subscribe(console.log)

Design-wise, we can't really create our own "pipe" that handles operators of the structure (source: Observable, ...args: any[]) => Observable<T>. Because there's not really an ergonomic way to allow this right now without a LOT of extra boilerplate for the consumer.

Proposed solutions/paths forward

Should the Hack pipeline be adopted, in order to leverage it, and get people slowly migrated to it, we're going to have to be creative.

Idea 1: Polymorphic operators

In this idea, we could have the operators detect whether or not the first argument to the operator function is an Observable, or perhaps implements Symbol.observable? Then behave accordingly.

basically:

// Hack signature
export function map<T, R>(source: Observable<T>, project: (value: T) => R): Observable<R>

// Pipeable signature:
export function map<T, R>(project: (value: T) => R): OperatorFunction<T, R>;

export function map<T, R>(sourceOrProject: Observable<T>|(value: T) => R, project?: (value: T) => R): OperatorFunction<T, R>|Observable<R> {
   if (isObservable(sourceOrProject)) {
      return originalMap(project)(sourceOrProject);
   } else {
      return originalMap(project);
   }
}


// "Standard" usage:
source$.pipe(
   map(x => x + x),
   map(x => x + '!!!')
)
.subscribe(console.log)

// "Hack" usage:
source$
  |> map(^, x => x + x)
  |> map(^, x => x + '!!!')
  |> ^.subscribe(console.log)
  
// Bonus? Operators can be used as "creation functions":

concatMap(source$, x => of(x, x, x)).subscribe(console.log);

Pros:

  • It's something we can easily do with a single function we apply everywhere
  • All "operators" can now be used in "static" ways. (We can probably also make the static functions behave as operators in a reverse manner).
  • concat and concatWith basically converge into a single concat (et al)

Cons:

  • We'd have to add a lot of additional typings
  • We're sort of doubling the API surface area by doubling the ways what we have can be used. And our API is already huge
  • It's possible to have runtime gaffs like this. For example someone accidentally passes an observable as the first argument. Although I think that TypeScript will save most people here.

Idea 2: Do nothing?

Leave our API as-is. People that want to use the new pipeline operator will have to call things like source$ |> map(fn)(%).

Pros:

  • WAY less work.

Cons:

  • Ugly. Probably won't fit into the "spirit" of pipeline operator use outside of RxJS.
  • We'll never get rid of having our our pipe method on Observable. Basically the pipeline feature that was added was useless to us.

Idea 3: Auto-Currying

Here we'd make sure that all of our operators were actually static functions that auto-curried, taking the source as the final argument. In other words:

import { of, map } from 'rxjs';

const source = of(1, 2, 3);

const fn = (x: number) => x + x;

// standard call
map(fn, source);

// calling with a functional pipe
source.pipe(map(fn))

// calling with the Hack pipeline
source |> map(fn, ^);

Pros:

  • Makes the RxJS library quite flexible. You'd be able to use all of our methods in a variety of ways.
  • Would also work with the F# pipeline if we're lucky enough to get that

Cons:

  • source argument being last, while consistent, is maybe unintuitive.
  • I'm unsure about the perf implications of wrapping everything in an auto-currying function. We'd have to play with that and see how that effects things.
  • Its possible to call an operator with a bad call pattern and get a function back, where maybe you should get an error.
  • There may be n-args operators that will not work well with this.
  • operators like combineLatestWith/combineLatest or withLatestFrom will end up changing the order of source's values in ways that may be very unintuitive. (e.g. a$.pipe(withLatestFrom(b$, c$)) // -> Observable<[b, c, a]>.)
@benlesh benlesh added the AGENDA ITEM Flagged for discussion at core team meetings label Aug 31, 2021
@josepot
Copy link
Contributor

josepot commented Aug 31, 2021

I haven't been following the discussions on this proposal, but I'm quite surprised (and disappointed) with the fact that the "F# like" proposal was ditched in favor of the "hack" proposal.

Personally, I would like to ask the RxJS core team not to spend any energies trying to promote/support this syntax 🙏. However, if the RxJS core team thinks that this syntax must be supported: what about if instead of creating this polymorphic overload, the library exports the operators that take the observable as the first argument from a separate package, as in:

import { map } from '@rxjs/hack-operators`;

?

Just an idea.

@benlesh
Copy link
Member Author

benlesh commented Aug 31, 2021

"F# like" proposal was ditched in favor of the "hack" proposal.

It's at the discretion of members of the TC39 and has little to do with what non-members want or even popular common usage. It's just what a paid member is willing to push forward, unfortunately, and the other proposal didn't have a champion from a member company.

Personally, I would like to ask the RxJS core team not to spend any energies trying to promote/support this syntax 🙏.

This is just a placeholder for ideas in case we're forced to support this. I wouldn't actually add this unless that proposal hit Stage 3 or 4.

@benlesh
Copy link
Member Author

benlesh commented Sep 9, 2021

Added auto-currying as an option. (See original post)

@tabatkins
Copy link

Reviewing these, I'll say that option 2 (do nothing) seems perfectly reasonable to me. The pipe() function can even result in slightly shorter code if there are several pipeline steps; the 2-char , between each step vs the 4-char |> between each will gradually overwhelm the difference between the 7 chars of .pipe() vs the initial |>. I also find pipe() quite readable on its own, myself, for what it's doing. (I come from a strong functional-programming background; I started as a Common Lisper and built my own utility library (don't make fun, I was young) of HOF operators cribbed from Haskell.)

That said, I understand the impulse to adapt to the pipe operator. Option 3 is definitely closest to your existing code, tho the incompatibility with n-ary functions will be problematic (currently you can rely on the "last argument" being specially distinguished from the rest of the args). The confusion of having the "most important" value (the Observable) show up last in the type line is also legit.

Option 1 is my personal favorite, and the option I'd intuitively reach for. As you note, it also allows for convenient usage of the functions independently. It also means that people adding their own functions have a slightly easier time if they're committed to only using |> for piping; they get to write a function in the ordinary style (taking all their arguments in the arglist) rather than starting it off with the return obs=>{...} boilerplate.

@dy
Copy link

dy commented Sep 11, 2021

2 cents from pipeline proposal#190 and jspipe: minimal F# pipes are possible today via defining valueOf on observables:

someObservable
  | map(x => x + x)
  | filter(x => x % 2 === 0)
  | concatMap(n => interval(n) | take(3))
  | subscribe(console.log)

It can be more reliable if/when operator overloading becomes a thing (it's a thing in QuickJS).

@kiprasmel
Copy link

kiprasmel commented Sep 11, 2021

"F# like" proposal was ditched in favor of the "hack" proposal.

@benlesh:

It's at the discretion of members of the TC39 and has little to do with what non-members want or even popular common usage. It's just what a paid member is willing to push forward, unfortunately, and the other proposal didn't have a champion from a member company.

How it is even possible to choose one of the competing proposals if nobody is defending the other one? wtf


My input on the F# > Hack: tc39/proposal-pipeline-operator#91 (comment)

@theScottyJam
Copy link

How it is even possible to choose one of the competing proposals if nobody is defending the other one?

There are people defending F# - we (the community) are :). We're also defending hack-style. We've laid out tons of arguments for both sides over the years in this repo. They read what we say and interact with us. They consider the arguments we lay out, along with the arguments from their peers, and then they make an informed decision based on all of this information. I don't think there needs to be someone in the comittee actively fighting for both sides to ensure both sides of the argument gets heard - that's the whole purpose of the public discussion on this repo.

@benlesh
Copy link
Member Author

benlesh commented Sep 12, 2021

Hey everybody. Can we please stay on topic? This issue is not about complaining about the Hack operator, this issue is about what we do if the Hack operator lands as-is. So unless your comment has something to do with that, I would very much like it if you kept it to yourself or posted it somewhere else. I appreciate that you have strong feelings about these things. I do too. But let's stay on topic.

@zakhenry
Copy link
Contributor

zakhenry commented Sep 14, 2021

I’m in favour of option 2 I.e. do nothing. But slightly stronger than that in that I think it should be recommended not to use the pipeline operator.

The hack style that appears to be moving forward makes a lot of use cases cleaner, but imo rxjs is not one of them.

I hope some day that we can go back to the dot chaining syntax that the early days of rxjs had before the performance implications were realised. A different change to the language along the lines of extension functions would needed to enable this.

@mikearnaldi
Copy link

My 2 cents on the proposals:

Idea 1: will have performance implications given that checking function parameters to discriminate which call was used has a cost

Idea 3: will have problems in typescript as having the source last without a pipe will make type inference start inferring from the wrong argument, i.e in your example map(fn, source) will use fn to infer making it impossible to so something like map(n => n + 1, source)

So I'd vote for idea 2 which is what we also opted for in the libraries I maintain

@benlesh
Copy link
Member Author

benlesh commented Sep 24, 2021

At this point, I think the core team is in agreement that the current pipeline proposal unfortunately isn't particularly useful for RxJS. We're going to opt for option 2, which is to do nothing and proceed as planned. Accommodating the operator won't really benefit this library much.

Us pivoting to support the Hack library with options 1 or 3 above will involve a lot of churn over a long amount of time both for us and for our users. I think we will serve our community best by not doing that. We will revisit this issue if the pipeline proposal nears Stage 4, if there is a high demand from the community to support it.

We will, of course, always be keeping an eye on new proposals as they come and go to see if there are things that will benefit the community. We will also, over time, pivot to make sure we're adapting to the platforms we support in the best and most ergonomic ways for our users.

@benlesh benlesh closed this as completed Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AGENDA ITEM Flagged for discussion at core team meetings
Projects
None yet
Development

No branches or pull requests

8 participants