tc39 / proposal-pipeline-operator

A proposal for adding a useful pipe operator to JavaScript.
http://tc39.github.io/proposal-pipeline-operator/
BSD 3-Clause "New" or "Revised" License
7.53k stars 108 forks source link

argue this proposal's design-pattern is less readable than using a temp-variable #173

Closed kaizhu256 closed 3 years ago

kaizhu256 commented 4 years ago

i argue this proposal is "un-pythonic" -- its an unnecessary design-pattern when temp-variables would suffice for most use-cases.

here are counter-examples to the ones in README.md using a tmp variable (which i find easier-to-debug). also, the pipeline-operator currently has no support for await, while temp-variables can do that easily:

Basic

// original
let result = exclaim(capitalize(doubleSay("hello")));

// pipeline-operator
let result = "hello"
  |> doubleSay
  |> capitalize
  |> exclaim;

// temp-variable
let tmp;
tmp = "hello";
tmp = doubleSay(tmp);
tmp = capitalize(tmp);
tmp = exclaim(tmp);
let result = tmp;

Functions with Multiple Arguments

// original
let newScore = boundScore( 0, 100, add(7, double(person.score)) );

// pipeline-operator
let newScore = person.score
  |> double
  |> (_ => add(7, _))
  |> (_ => boundScore(0, 100, _));

// temp-variable
let tmp;
tmp = person.score;
tmp = double(tmp);
tmp = add(7, tmp);
tmp = boundScore(0, 100, tmp);
let newScore = tmp;

Use of await

// original
asyn function foo(isostring) {
  return await (
    await fetch(
        "https://foo.com?" + new Date(isostring).getTime()
    )
  ).json();

// pipeline-operator
???

// temp-variable
asyn function foo(isostring) {
    let tmp;
    tmp = isostring;
    tmp = new Date(tmp).getTime();
    tmp = await fetch("https://foo.com?" + tmp);
    tmp = await tmp.json();
    let result = tmp;
}

Usage with ? partial application syntax

// original
let newScore = boundScore( 0, 100, add(7, double(person.score)) );

// pipeline-operator
let newScore = person.score
  |> double
  |> add(7, ?)
  |> boundScore(0, 100, ?);

// temp-variable
let tmp;
tmp = person.score;
tmp = double(tmp);
tmp = add(7, tmp);
tmp = boundScore(0, 100, tmp);
let newScore = tmp;

Transforming Streams/Iterables/AsyncIterables/Observables

// original
???

// pipeline-operator
numbers()
  |> filter(x => x % 2 === 1)
  |> map(x => x + x)

// temp-variable
let tmp;
tmp = numbers();
tmp = filter(tmp)(x => x % 2 === 1);
tmp = map(tmp)(x => x + x);
let result = tmp;

Object Decorators

// original
???

// pipeline-operator
function Person (name, age) {
  return { name: name } |> greets |> ages(age);
}
function Programmer (name, age) {
  return { name: name }
    |> greets
    |> ages(age)
    |> programs("javascript");
}

// temp-variable
function Person (name, age) {
  let tmp;
  tmp = { name: name };
  tmp = greets(tmp);
  tmp = ages(age);
  return tmp;
}
function Programmer (name, age) {
  let tmp;
  tmp = { name: name };
  tmp = greets(tmp);
  tmp = ages(age);
  tmp = programs("javascript");
  return tmp;
}

Validation

// original
???

// pipeline-operator
function createPerson (attrs) {
  attrs
    |> bounded('age', 1, 100)
    |> format('name', /^[a-z]$/i)
    |> Person.insertIntoDatabase;
}

// temp-variable
function createPerson (attrs) {
  let tmp;
  tmp = attrs;
  tmp = bounded('age', 1, 100)(tmp);
  tmp = format('name', /^[a-z]$/i)(tmp);
  Person.insertIntoDatabase(tmp);
}

Usage with Prototypes

// pipeline-operator
getAllPlayers()
  .filter( p => p.score > 100 )
  .sort()
|> (_ => Lazy(_)
  .map( p => p.name )
  .take(5))
|> (_ => renderLeaderboard('#my-div', _));

// temp-variable
let tmp;
tmp = getAllPlayers()
  .filter( p => p.score > 100 )
  .sort();
tmp = Lazy(tmp)
  .map( p => p.name )
  .take(5));
renderLeaderboard('#my-div', tmp);

Mixins

// original
class Comment extends Sharable(Editable(Model)) {
  // ...
}

// pipeline-operator
class Comment extends Model |> Editable |> Sharable {
  // ...
}

// temp-variable
let Tmp;
Tmp = Model;
Tmp = Editable(Tmp);
Tmp = Sharable(Tmp);
class Comment extends Tmp {
  // ...
}
mpcarolin commented 4 years ago

Regarding the await statement, both the F# and smart pipeline proposals suggest ways of handling this. You are correct that the minimal proposal does not, but await support is nonetheless planned, even if the details are undecided.

Regarding readability, this is somewhat subjective, but I don't agree that this approach is generally more readable. It requires more repeated syntax and an additional variable than necessary. I do see an argument in favor of its readability with partial application, though.

Additionally, this approach introduces additional state to propagate the pipeline via the temp variable. The proposed operator and the status quo, on the other hand, do not.

mAAdhaTTah commented 4 years ago

The streams/iterables temp variable version is incorrect, which undermines your argument quite a bit.

kaizhu256 commented 4 years ago

@mAAdhaTTah, yea as mid-tier programmer i'm admittedly terrible at deciphering logic-flow of function-generators in javascript. would this be the correct vanilla js interpretation?

// pipeline-operator
numbers()
  |> filter(x => x % 2 === 1)
  |> map(x => x + x)

// temp-variable
let tmp;
tmp = numbers();
tmp = filter(x => x % 2 === 1)(tmp);
tmp = map(x => x + x)(tmp);
let result = tmp;

also the fact average javascript-programmer like me has trouble figuring out how arguments are passed around in pipeline-operators speaks a little about its "tech-debt-ness" in real-world code (and also function generators ;)

mAAdhaTTah commented 4 years ago

But you wrote the pipeline code right the first time. You clearly knew what the code was doing and how it worked. It's just harder to write the temp variable version. Pipeline is an improvement.

kaizhu256 commented 4 years ago

no, i don't know if the pipeline code worked or not. i just copy-pasted README.md examples verbatim. for all i know the streaming example in README.md might be incorrect.

again i'm terrible at troubleshooting bugs of function-generators and the "magicness" of pipeline-operator makes that task even harder.

ljharb commented 4 years ago

Either way:

kaizhu256 commented 4 years ago

@ljharb, "readable" might have been the wrong term. "easier-to-debug" would be the better term. i do agree pipeline-operators look sexy, but which of the following coding-styles would you prefer to debug-and-fix if an issue arised in production?

// pipeline-operator
let newScore = person.score
  |> double
  |> (_ => add(7, _))
  |> (_ => boundScore(0, 100, _));

// temp-variable
let newScore = person.score;
newScore = double(newScore);
newScore = add(7, newScore);
newScore = boundScore(0, 100, newScore);

for me, the latter is easier to inject print-statements and modify/refactor its transformation.

jrista commented 4 years ago

The idea behind a proper pipeline is that you can do away with variable assignment. I use a pipe() function these days, and I've effectively eliminated variable assignment in my code, achieved purity in the vast majority of my functions, and leverage immutable data throughout most of my code.

Variables, especially reusing a temp variable, violates a lot of the tenets of functional programming.

Regarding debuggability...a lot of that has to do with how the debuggers work. With proper tooling support, debugging the pipe code should be just as easy as the temp var code.

Regarding logging, easy enough with a pipe and a very simple curried log function:

const log = msg => value => console.log(msg, value);

let newScore = person.score
  |> double
  |> log('after double')
  |> (_ => add(7, _))
//  |> log('after add')
  |> (_ => boundScore(0, 100, _))
  |> log('after boundScore');
ljharb commented 4 years ago

@kaizhu256 the debugger would stop at each point in the pipeline, so i'd vastly prefer to debug the pipeline example.

The way I debug modern applications is not primarily by modifying code, but by using the debugger.

kaizhu256 commented 4 years ago

The way I debug modern applications is not primarily by modifying code, but by using the debugger.

its common for me to ssh into machines/devices where the only "ide" available is vim. ease-of-debugging in minimal-tooling environments shouldn't be discounted when discussing ergonmics of a "scripting" languages like javascript.

ljharb commented 4 years ago

On those machines, I presume node --inspect would also be available? I wasn't recommending IDEs, as I never use those.

kaizhu256 commented 4 years ago

hmm, just discovered ssh-tunnelling node --inspect-brk. thx for suggestion and kinda see your point.

https://nodejs.org/en/docs/guides/debugging-getting-started/#enabling-remote-debugging-scenarios

phaux commented 4 years ago

That would fail to typecheck because you're assigning different type again and again to the same variable.

kaizhu256 commented 4 years ago

That would fail to typecheck because you're assigning different type again and again to the same variable.

that's exactly the kinda bias coming from ppl with staticallly-typed language backgrounds.

you guys/girls can't even comprehend that temp-variables is equally valid coding-style in dynamically-typed javascript, just because its not possible in statically-typed haskell/c#/java etc.

noppa commented 4 years ago

FWIW the temp variable pattern actually type checks just fine in both TypeScript (playground) and Flow (playground)
(In TS, the trick is to decalre let tmp; separately before assigning to it).

IMO the pipeline operator just looks better and more clearly conveys the intent. We have had temp variables forever, but (based on my subjective experience) they are not commonly used like this and JS developers often reach for other solutions like pipe helper functions instead. Seems to me that if the temp variable pattern were sufficient, it would already be in use.

kasperpeulen commented 4 years ago

We have had temp variables forever, but (based on my subjective experience) they are not commonly used like this and JS developers often reach for other solutions like pipe helper functions instead.

Exactly, probably more common, for people with OOP background, is to make some "fluent API". Wrapping something in a class (or object of methods), just so that you can chain method after method.

For example

const set = new MySetHelper(set)
  .map(...)
  .filter(...)
  .getSet()

However, I think many new OOP languages, such as C#, Swift and Kotlin implemented extension methods, as wrapping values in classes, just to make a fluent API, becomes quite tedious, and for any method that any user will ever want, you will get an issue on your tracker, while it may have made more sense if the user implemented that method itself.

Also it doesn't compose very well,

For example:

const text = new MyStringHelper(
    new MySetHelper(set)
      .map(...)
      .filter(...)
      .reduce(...)
  ).capitalize()).getString();

In the case of javascript, another problem is that because of tree shaking only working for top level exported functions, exposing your API in this way, bloats your bundle size.

That is why Rx changed their approach, to using top level curried functions for their operators.

Actually, a mega library like jquery, could have a fluent API using the pipeline operator, that is perfectly tree shakable and slim.

jrista commented 4 years ago

That would fail to typecheck because you're assigning different type again and again to the same variable.

that's exactly the kinda bias coming from ppl with staticallly-typed language backgrounds.

you guys/girls can't even comprehend that temp-variables is equally valid coding-style in dynamically-typed javascript, just because its not possible in statically-typed haskell/c#/java etc.

It really isn't about static typing. It is about writing safe, reliable code. This applies to dynamic languages just as much as any other, and it comes from functional programming background.

Mutations, while certainly "valid", are also highly problematic and more easily lead to bugs.

alamothe commented 3 years ago

So would we have a single temp variable per function? Or have tmp1, tmp2, tmp3...? Not to mention that variables leak unless you scope them. This workaround breaks down at the slightest attempt to scale.

kaizhu256 commented 3 years ago

it scopes and scales fine with a linter, which is standard practice these days.

qzb commented 3 years ago

FWIW the temp variable pattern actually type checks just fine in both TypeScript (playground) and Flow (playground) (In TS, the trick is to decalre let tmp; separately before assigning to it).

@noppa I don't know how it works in Flow, but in Typescript it simply fallbacks to any type. So it complies, but in practice you are loosing advantages of having type checks.

ljharb commented 3 years ago

A linter is indeed standard practice in JS, for well over a decade now, but I don't think that justifies making the code less readable by adding temp variables.

noppa commented 3 years ago

@qzb I don't think that's what's happening here: Playground example.
In this example, TS correctly infers the tmp variable to be string, not any, in the last step, even though we went through multiple different types to get there.

  let tmp;
  tmp = [1.123, 2.123, 3.123]
  tmp = map(tmp, _ => _.toFixed(2))
  tmp = find(tmp, _ => _ === '1.12')
  tmp = tmp?.charAtttt(0)

[TS]  Property 'charAtttt' does not exist on type 'string'.

I'll re-emphasize that I'm personally not in favor of the temp variable pattern. Just pointing out that it can be type-checked.

qzb commented 3 years ago

@noppa Why I didn't knew about that? It makes easier so many things... 😅

nicolo-ribaudo commented 3 years ago

To be fair, that example also works in Flow: https://flow.org/try/#0CYUwxgNghgTiAEAzArgOzAFwJYHtXwFsoAHAHgBUAaeAJQD4AKWGALngBIaQpgB5VCAE8AgjBhRBFOtSwYQ4uSDYMAblAhtyASngBeOrS1saAbQC6AKFCRYCFOmx4kWVMClMxbTtz4CRYiSlqYjhgLDAoOWU1DXhtPQMAIxwcCG5UI3gVHCxgeAAfOIsLe0xcfCIXBh0Abwt4eDSMeAwCYgBuepa2vXgTAEYAOn6AJgBmahHh8eox6bHLBtbiXqJiBmXqAH0E+C3BjBwAMSwADxBgBhGtLS7l3sQXS8293Z3dD-gAciHRr9ulj1dN1iAB+QZgAAWsGEGDhGAYAAZbl04BhkDB8MsLABfYpgPAAZ2acmJbGyuQK8GJMBcAHNVlAqlogA

However, both Flow and TypeScript can correctly refine the type of tmp inside the pipeline but they don't keep the correct type after the pipeline (if you annotate main() as returning number in TS or void | number in Flow you won't get an error). (EDIT: I did something wrong, see https://github.com/tc39/proposal-pipeline-operator/issues/173#issuecomment-728892475)

And same as @noppa, I think that tmp variables are less readable than pipelines (tmp vars are just noise imo).

noppa commented 3 years ago

@nicolo-ribaudo We are getting a bit offtopic here but I have to ask what you mean by

if you annotate main() as returning number in TS or void | number in Flow you won't get an error

Seems both Flow and TS give an error if you annotate void/undefined | number as the return type instead of void/undefined | string and try to return tmp.

nicolo-ribaudo commented 3 years ago

Oh I meant that when I tried with code similar to yours they didn't report an error, I'm not sure about what I tested in their playgrounds :sweat_smile:

I have edited my previous comment, and we should hide this one and your last one since they are definitely off-topic.

defims commented 3 years ago

It seems that using an IIFE to wrap the temp variable expression is more similar to pipeline operator.

Basic

// original
let result = exclaim(capitalize(doubleSay("hello")));

// pipeline-operator
let result = "hello"
  |> doubleSay
  |> capitalize
  |> exclaim;

// temp-variable
let tmp;
tmp = "hello";
tmp = double(tmp);
tmp = add(7, tmp);
tmp = boundScore(0, 100, tmp);
let newScore = tmp;

// IIFE
let result = (_ => (_= "hello"
  ,_= doubleSay(_)
  ,_= capitalize(_)
  ,_= exclaim(_))()

Functions with Multiple Arguments

// original
let newScore = boundScore( 0, 100, add(7, double(person.score)) );

// pipeline-operator
let newScore = person.score
  |> double
  |> (_ => add(7, _))
  |> (_ => boundScore(0, 100, _));

// temp-variable
let tmp;
tmp = person.score;
tmp = double(tmp);
tmp = add(7, tmp);
tmp = boundScore(0, 100, tmp);
let newScore = tmp;

// IIFE
let result = (_ => (_= person.score
  ,_= double(_)
  ,_= add(7, _)
  ,_= boundScore(0, 100, _))()
bjunc commented 3 years ago

@kaizhu256 In my experience with Elixir, it would be a very rare scenario to have a pipeline with many functions that don't expect the preceding returned value as the 1st arg; which makes your subsequent points a bit of a strawman. In reality, it would look more like this:

foo
|> bar()
|> (returnedBarValue => oops(42, returnedBarValue))
|> baz()
|> qux()

Additionally, I don't see any reason for a "temp" variable. The returned value is simply the first argument in the next function (in this case, an anonymous function that wraps the oops function so you can use the preceding value as the 2nd arg).

ghost commented 3 years ago

A question to ask people who think temp variables are more readable than a pipeline is whether they think temporary variables are more readable than fluent interfaces.

// Common industry practice
 const maleNameList = users.filter(user => user.gender === 'male')
                           .map(user => user.givenName)
                           .join('\n')

// Would you rather do this?
const males        = users.filter(user => user.gender === 'male')
const maleNames    = males.map(man => man.givenName)
const maleNameList = maleNames.join('\n')

// Or even this? Why?
let tmp;
tmp = users;
tmp = tmp.filter(user => user.gender === 'male');
tmp = tmp.map(user => user.givenName);
tmp = tmp.join('\n');
const maleNameList = tmp;
kaizhu256 commented 3 years ago

@alnj i agree dot-chaining is the correct design-pattern for arrays and strings due to how refined their api's are in javascript from heavy-usage. pipeline-operators bring zero-value to most array/string transformations.

and pretty everything else besides arrays/strings should be using a topic/tmp variable:

let data;
data = "https://www.example.com/api/foo";
data = await fetch(data);
data = await data.json();
data = data.tagList || [];
data = data.map(function (elem) {
    return elem.name;
}).filter(function (name) {
    return name.length > 3
}).sort().slice(0, 20);
data = ...

in above example, again, replacing it with pipeline-operators would bring zero readability or ease-of-code-refactorability benefits.

ghost commented 3 years ago

I think that those real-world examples are much more readable than what their temp variable equivalent would look like.

await in pipelines is a special case. You're right to bring up the potential problems with using it inside a pipeline, but there are many other contexts where a pipeline operator is valuable.

dot-chaining is the correct design-pattern for arrays and strings due to how refined their api's are in javascript from heavy-usage

Many projects' functional APIs are plenty refined enough to benefit from piping.

samhh commented 3 years ago

@kaizhu256 A pipeline operator gives you one less thing to think about versus that example. You know with a pipeline that the value is piped through the functions, and only those functions. WIth your example there are many other ways in which data could be modified, including halfway through your series of statements. Your example also reintroduces all the usual criticisms of using let over const. You as the author may be confident that you're using your "temporary variable" to mimic a pipeline, but I tomorrow will not; I will have to mentally keep track of all the different ways in which this approach could produce different results.

In my view a lot of good language design is constraining what's possible to make things easier to reason about and mistakes harder. This is partially why I favour immutability over mutability (purity over impurity more broadly) and static typing over a lack thereof. The same applies to function composition and pipeline application.

theScottyJam commented 3 years ago

You can think of the pipeline operator simply as a more functional alternative to fluent APIs.

If Javascript had the pipeline operator from the beginning, then our array-manipulation functions might have been designed as static methods instead of instance methods, so that they would fit well into a pipeline. Many other languages that have a pipeline operator design their array functions this way.

 const maleNameList = users
  |> Array.filter(?, user => user.gender === 'male')
  |> Array.map(?, user => user.givenName)
  |> Array.join(?, '\n')

This makes it very easy to add your own, home-baked array helper functions into the pipeline (something that can't be done with a fluent API)

 const maleNameList = users
  |> Array.filter(?, user => user.gender === 'male')
  |> Array.map(?, user => user.givenName)
  |> filterOutNullish(?)
  |> Array.join(?, '\n')

While it's too late to design our array methods this way (a shame), future APIs can still benefit from this pipeline syntax.

And, who knows, maybe some proposal down the pipeline would make it very easy to use instance methods in a pipeline. They are, after all, available as static methods on the prototype - we would just need a shorthand to do Array.prototype.someFunction.call(yourArray, ...), and then any fluent-API could be used in a pipeline instead, and reap the same benefits. But - I doubt that would ever happen.

As an aside, No one's rooting for this pipeline operator because it adds extra power to the language, in fact, it's quite the opposite. People love the pipeline operator precisely because it's constraining. At a glance, it's very easy to know what a pipeline is doing - it's taking the previous value, injecting it into the next line, and so forth. That's all it can do, and all it'll ever do. People are so against the "temp variable" idea because it's way too flexible. You have to look at every line to understand that you're just feeding the temp variable through as if it were a pipeline, and you have to look at the rest of the function definition to make sure you're not using the final temp variable anywhere else because that's very possible. You also have to make sure that no extra variables are being created in the middle of that "temp variable pipeline" that might get used elsewhere. It takes a lot more work to read code using a temporary variable - especially when it's in the middle of a longer, cluttered function.

This is a similar reason why "goto" has been abandoned in most languages - it's way too powerful and makes code harder to reason about (though goto is a lot worse than temp variables). It's why constructs such as const has been added to Javascript - it doesn't add any power to the language, rather, it enforces certain restrictions that help make the code easier to read.

ghost commented 3 years ago

@theScottyJam You should be able to chain fluent interfaces and pipeline operators together as long as they return the same data type. (example using F# syntax + partial application).

 const maleNameList = users.filter(user => user.gender === 'male')
                           |> Array.prototype.map.call(?, user => user.givenName)
                           |> filterOutNullishFromArray
                           .join('\n')

edit: After trying it out, the example above is wrong. ^^' It would probably have to be something like this.

 const maleNameList = users.filter(user => user.gender === 'male')
                           |> Array.prototype.map.call(?, user => user.givenName)
                           |> filterOutNullishFromArray
                           |> arr => arr.join('\n')
ducaale commented 3 years ago

FYI With a little bit of magic, it is possible to come up with a nice API for using pipelines with native prototype functions

const pipeable = (class_) => new Proxy({}, {
  get: (target, prop) => (
    (prop in class_.prototype)
      ? (...args) => (receiver) => class_.prototype[prop].call(receiver, ...args)
      : class_[prop].bind(class_) // https://stackoverflow.com/a/30819436/5915221
  )
});

const Array = pipeable(globalThis.Array)

const x = [1, 2, 3, 4, 5, 6, 7, 8, 9]
  |> Array.map(n => n * 2)
  |> Array.filter(n => n > 10)
  |> Array.join('-')

See https://gist.github.com/ducaale/93bd6d49314ef1383f50be95edca9d6e

theScottyJam commented 3 years ago

Another option would be the following, if the bind proposal goes through.

const { filter, map, join } = Array.prototype

const maleNameList = users
  |> ?::filter(user => user.gender === 'male')
  |> ?::map(user => user.givenName)
  |> ?::join('\n')

I'm not necessarily saying it can't be done, I'm just hesitant about actually doing it in production code - just using the array methods as a fluent API will make my code much easier to understand by more people, without sacrificing too much (in most cases). Maybe in personal projects, I would be more inclined to pick these functions off of the prototype.

tabatkins commented 3 years ago

The proposal has advanced to stage 2 with Hack syntax, so I'm closing this issue. (Also, @theScottyJam's response excellently describes my thoughts.)