Skip to content
This repository has been archived by the owner on Mar 25, 2018. It is now read-only.

NG should be idiomatic JavaScript #14

Open
CrabDude opened this issue May 11, 2015 · 31 comments
Open

NG should be idiomatic JavaScript #14

CrabDude opened this issue May 11, 2015 · 31 comments

Comments

@CrabDude
Copy link

With ESNext, there's a whole suite of new functionality previously provided by core. Given the opportunity for breaking changes, NG should strive to be idiomatic JavaScript in all regards (EDIT: Language features are not de facto idiomatic JavaScript, but they are likely candidates):

Some obvious candidates:

  • All async core APIs should use async
  • Deprecate require in favor of import
  • Deprecate errbacks
  • async/await focused documentation
  • Use of class for EventEmitter, ReadableStream, etc...

Distant future candidates:

  • Extend Streams to be implementations of Observables or AsyncIterators
    • Would this mean the Stream API should be deprecated?
    • What benefits does the Stream API offer beyond proposed language equivalents

I'm sure there are other new language features that could be added to the list. Also, for the sake of discussion, assume a stable performant finalized implementation exists for all of the above in V8 without a flag.

EDIT:

  1. s/callbacks/errbacks/
  2. My desire here is that NG would focus on where it adds value beyond the language features as has always been the philosophical stance of core. For instance, there is no sense wasting effort maintaining an alternative Stream API if it objectively offered no benefit over a language equivalent. That said, neither core APIs nor language APIs should be assumed to be superior by default.
@Qard
Copy link
Member

Qard commented May 11, 2015

👍 for all of this.

I worry a bit about NG becoming the new Python 3, with such drastic changes on the table. But I think the ES6 way is so clearly better that it hopefully won't come to that.

@CrabDude
Copy link
Author

@Qard It at least has the benefit of not having to solve these problems itself. Others will both solve and experience the pain first.

@Qard
Copy link
Member

Qard commented May 11, 2015

We also have semver, so there's that.

@bmeck
Copy link
Member

bmeck commented May 11, 2015

-1 callbacks
need more info, but I will probably just start to giggle if I hear Promises instead.

-1 async await
Wait on cancellation mechanism prior to jumping the gun, generator.return / a controller seems to be the path that is picking up the most steam, but it is not going anywhere in the near future.

-1 to classes
Mixins / EventEmitter don't work with classes because classes use single inheritance, don't just use classes because they are new and shiny. Same problem with Readable/Writable (how can you have both if each is a class 😲 ).

@vkurchatkin
Copy link

All async core APIs should use async

No need to use, it's enough to return promises

async/await focused documentation

Too early for that. async/await is hardly idiomatic JavaScript, it's not JavaScript at all (yet)

Use of class for EventEmitter, ReadableStream, etc...

This works already, you can extend those with ES6 class syntax.

@Qard
Copy link
Member

Qard commented May 11, 2015

This is NG we're talking about, not mainline. If something looks like a thing that could be a solution in the future, it should be discussed. This is not an absolute task list.

Also, async/await is a fairly developed proposal, which people are already using via transpilers, so I think it is quite likely it will become a thing people use regularly soon.

@bmeck
Copy link
Member

bmeck commented May 11, 2015

@Qard yes but some people w/ experience using async/away are moving to compositional functions due to issues. Those issues are still in question and are not getting traction to my knowledge.

As for the other stuff, moving to some things does not make sense, even if it is possible it might not be a good idea.

@juliangruber
Copy link
Member

using promises is safe and sound at the moment, and since they will be able to be awaited that sounds like a no brainer to me

@boopathi
Copy link

Extend Streams to be implementations of Observables

We also have https://github.com/zenparsing/async-iteration .

@mikeal
Copy link
Contributor

mikeal commented May 11, 2015

I don't like the way this is being framed.

First off, just because a feature in landing in the language doesn't mean it becomes "idiomatic" JavaScript. Is with idiomatic? Is freezing objects idiomatic? You could argue that even strict mode hasn't become idiomatic yet.

What is idiomatic is whatever the community adopts. This puts the platform in an odd position because it sets patterns that will likely spur adoption of whatever features we decide to adopt. If these patterns gain a lot of traction in the frontend we can safely assume they will be idiomatic once they are also supported well in the platform but my guess is that we'll end up leading here rather than following.

You'll recall that a sizable community spun up around generators and is now slowly dying off with even some of its proponents now opting for promises + async/await. Still, that was a feature in the language, people may have even started to call it idiomatic, and if we had implemented it in core we would have been wrong to do so.

Honestly, I'm cautious of any pattern that doesn't have a dozen or so articles talking about where it sucks and the problems it can cause. Any pattern or abstraction if you use it enough will have issues, deciding which ones are more or less important and understanding the tradeoffs is how you make good decisions. Because some of these features are still in draft specs and cross compilers the people adopting them widely have a high tolerance for living on the bleeding edge and so it'll be some time before people more honestly talk about the warts and we can have a good discussion about the tradeoffs.

A good example of this is promise errors. I could only get promise supporters to admit the issues promises have with errors (silent failures being quite common) in dark rooms in private for years. Nobody had written an article about this that I had seen, it was just this dirty little secret. It wasn't until v8 supported them natively and we had them in io.js did I see people discuss this issue honestly and, very quickly, a solution was proposed, implemented and shipped. It just shows that we need time with these features, not just in the ecosystem through cross compilers, but "native" in the platform even if not in use by the standard library, before we understand them enough to discuss them honestly and evaluate the tradeoffs.

@mikeal
Copy link
Contributor

mikeal commented May 11, 2015

That all said, Promises appear to be well on their way to being "proven." The userland promise modules are some of the most depended on modules in npm, frontend tooling has started adopting them widely, and the native implementation in io.js has been progressing nicely. If the performance and debugging issues can be dealt with in v8, and @domenic is confident they can be, they'll need to be a big part of any future platform.

I still hate them personally, but I'm in the minority and even I can't ignore how much better they've gotten.

@bmeck
Copy link
Member

bmeck commented May 11, 2015

@mikeal I would love if we had a v8 vm implementor comment prior to talking
about promise perf, I have seen mixed reviews on if optimization is
possible.

On Mon, May 11, 2015 at 11:22 AM, Mikeal Rogers [email protected]
wrote:

That all said, Promises appear to be well on their way to being "proven."
The userland promise modules are some of the most depended on modules in
npm, frontend tooling has started adopting them widely, and the native
implementation in io.js has been progressing nicely. If the performance and
debugging issues can be dealt with in v8, and @domenic
https://github.com/domenic is confident they can be, they'll need to be
a big part of any future platform.

I still hate them personally, but I'm in the minority and even I can't
ignore how much better they've gotten.


Reply to this email directly or view it on GitHub
#14 (comment).

@domenic
Copy link

domenic commented May 11, 2015

We have had some internal conversations about promise perf, and though I don't want to quote directly since they were internal, I can say that the consensus is that the current implementation is written without any attention to performance, that there's a lot of low-hanging optimization fruit, and that with some effort it should be trivial to meet or exceed Bluebird-level performance.

@CrabDude
Copy link
Author

@mikeal I hear you and echo most[1] of your perspective.

This issue is not about language features being de facto idiomatic. It's about avoiding a node-flavored versions when language-first versions are (will be) adopted by the majority (assumption) of the community. I've added an "Edit" to the OP to clarify. 1st class language implementations are typically superior in that they tend to be faster and widely adopted. class is an excellent example of what I'm arguing for here...

Despite the backlash against class and its preference for inheritance over composition (see Gang of Four for the counter) the proposal for class here is tangential. Streams inherit from EventEmitter, and sugaring and "knowing what's going on under the hood" arguments aside, it's far cleaner and reader/learner-friendly to write:

class CapsLockStream extends Transform {
  constructor() {
    super()
  }

  _transform(data, encoding, done) {
    for (let i = 0; i < data.length; i++) {
      if (data[i] >= 97 && data[i] <= 122) data[i] &= ~32
    }
    this.push(data)
    done()
  }
}

instead of...

function CapsLockStream() {
  Transform.call(this)
}

CapsLockStream.prototype._transform(data, encoding, done) {
  for (let i = 0; i < data.length; i++) {
    if (data[i] >= 97 && data[i] <= 122) data[i] &= ~32
  }
  this.push(data)
  done()
}
util.inherits(CapsLockStream, Transform)

Teaching the latter over the former to new developers is a fool's errand, and I suspect the experts (meant sincerely) that are core developers are years or decades removed from the mental hurdle of understanding function constructors, a hurdle that is largely responsible for the node idiomatic util.inherits. Removing this hurdle is the motivation for the proposal to use class and encourage class for extension in the documentation.

If this were Io, one could argue it's "too early" or "backwards breaking," and it would be a recurring frustrating refrain, but this is NG. Biases should be put aside in pursuit of objective bests. Assuming community adoption, in the case of class, I see no benefit in continuing to encourage the use of the antiquated function constructor.

This issue is largely to encourage and discuss rebuttals if they exist.

To avoid being "born derailed," please note no mention of Promise in the OP. Deprecating errbacks is flirting with that, but errbacks as a paradigm are an anti-pattern[2] doomed to antiquity with both no adoption and pervasive derision outside the node community. More importantly, for new node.js engineers, async/await obliterates the async programming learning curve, and it is my motivation for NG to thrive and grow through the adoption of simpler paradigms and the removal of inconsistency and idiosyncrasies that hinder such.

That said, are there any arguments for require's superiority over import?

@bmeck

some people w/ experience using async/await are moving to compositional functions

I'm ignorant here. Could you elaborate?

how can you have both [Readable/Writable] if each is a class

This is a special case, and the documentation favors extending Transform or Duplex, illustrated in the above example.

-1 callbacks... need more info

See [2] below.

cancellation mechanism

This seems like a straw-man(?). Neither callbacks nor promises are cancelable. The proposition is that async/await is superior in every way.

[1] I don't know of anyone that seriously believed async generators to be anything other than an incredibly useful hack. They were far from idiomatic in that they were neither the language's intended purpose for their use, nor had the asynchrony (yieldable) yet settled on Promise.

[2] For those who know me, I was vehemently anti-Promise for the first 3-4 years of node.js programming, and only after my many experiences building node applications, teaching thousands of developers, and attempting to solve async error handling with trycatch did I recognize the pernicious trappings of the callback anti-pattern. Only after the adoption of Promise and async/await into the language did I reluctantly realize their multi-fold benefit. I was also a persistent proponent against them for reasons like silent failure and errors vs exceptions, but find the failings in callbacks to be worse.

@bmeck
Copy link
Member

bmeck commented May 11, 2015

Compositional functions : https://github.com/jhusain/compositional-functions

Basically complex workflows have struggled / not help up w/ Promises as the backing controller for async await for various people.

This is a special case, and the documentation favors extending Transform or Duplex, illustrated in the above example.

The main issue here is you cannot have something be both instanceof Readable and instanceof Writable, what gains are there from having the root be a class? I am fine w/ 3rd parties extending things, but for the root where classes cannot represent the structure we should not force them in.

This seems like a straw-man(?). Neither callbacks nor promises are cancelable. The proposition is that async/await is superior in every way.

The problem however comes from a matter of overall gains vs losses, I tried to move to async/await and lockfile usage and lack of abort made me make my own library to work w/ a more flexible system. I am completely against promises as control flow; in particular without having a cancellation mechanism / finally{} mechanism. In a callback based system we can wrap functions in a if(aborted) guard since they are composable pieces of a task. In async/await we cannot as it stands today.

Lets take a moment to see how we use cancellation at work, but have had to avoid async/await to do so:

let task = function* (abort) {
  let release = yield lockfile('filename.lock', abort);
  try {
     db.query(...);
     fs.write('filename', ...);
     ...
  }
  finally {
     release();
  }
}
  • Are there better solutions than callbacks, yes.
  • Do I think async/await is a syntactically particularly useful one, yes.
  • Do I think it is one that is ready for adoption, no.
  • Is this related to Promises, only due to async/await enforcing Promises and the lack of cancellation semantics (potentially could be solved by allowing other data structs).

@rlidwka
Copy link

rlidwka commented May 11, 2015

I should probably write a browser plugin that replaces word idiomatic with my personal preference in this entire thread.


1st class language implementations are typically superior in that they tend to be faster and widely adopted.

Not really. See "native promises vs bluebird" above. In fact, all new es6 features tend to be slower than old equivalents, because nobody bothers to optimize them just yet.

Streams inherit from EventEmitter, and sugaring and "knowing what's going on under the hood" arguments aside, it's far cleaner and reader/learner-friendly to write:

It is also far cleaner and reader/learner-friendly to write javascripts without semicolons.

That's the same level of purely style-related bikeshedding. Not that I'm against that, but still.

a hurdle that is largely responsible for the node idiomatic util.inherits.

util.inherits(Foo, Bar) should probably be replaced with Object.setPrototypeOf(Foo.prototype, Bar.prototype) or Object.assign(Foo.prototype, Bar.prototype). Because util.inherits right now is just redundant.

That said, are there any arguments for require's superiority over import?

AFAIK, import is static keyword resolved before the script loads, and require is a run-time function. Two different things. For example, you can't do this with import:

fs.readdirSync(__dirname).forEach((file) => {
  module.exports[file] = require(`#{__dirname}/#{file}`)
})

Though some people argue you shouldn't do it with require either.

I don't know of anyone that seriously believed async generators to be anything other than an incredibly useful hack.

I do seriously believe that async generators are superior to promises and callbacks right now. And I do have a private project written purely in async generators, even though public ones are still using promises/callbacks for legacy reasons.

I was also a persistent proponent against them for reasons like silent failure and errors vs exceptions, but find the failings in callbacks to be worse.

I don't believe anything could be worse than silent failure. If your process crash, you could notice and fix it, but if your process just ignores errors and stops executing midway, there's nothing worse than that.

Fortunately, recent introduction of unhandledRejection in io.js mitigated it somehow, so ES6 promises now at least useable.

Callbacks on the other hand work fine and always did. Only disadvantage is they don't work well with high-order functions (e.g. when you monkey-patch a generic async function, you need to do a rather complex stuff to get a position of a callback in arguments array), thus they don't play well with promises/thunks/a-generators/etc.

I wish io.js used thunks instead of callbacks, like this:

fs.readfile('/tmp/foo')(function (err, res) { ... })

It would be the same idea as promises, but without needless .then/.catch cruft.

@vkurchatkin
Copy link

AFAIK, import is static keyword resolved before the script loads, and require is a run-time function. Two different things. For example, you can't do this with import

There is loader API for this.

@chrisdickinson
Copy link

@CrabDude NB, your second stream example is not quite accurate – we recently grew WHATWG-style constructors:

const txf = new Transform({
  transform(data, enc, ready) {
    return ready(null, data.embiggen());
  }
});

I would heartily suggest avoiding teaching newcomers that inheritance is the proper way to interact with the streams API – and even more emphatically suggest avoiding subclassing event emitters.

@CrabDude
Copy link
Author

@bmeck @rlidwka @chrisdickinson @vkurchatkin =) Lots of constructive feedback makes me happy.

@chrisdickinson

I would heartily suggest avoiding teaching newcomers that inheritance is the proper way to interact with the streams API

I'm not crazy about inheritance, and prefer the simpler let t = new Transform; t._transform = ..., but the alternative is to teach them function constructors, which require far more mental overhead.

WHATWG-style constructors

Unfamiliar. Reference?

@bmeck

Compositional functions : https://github.com/jhusain/compositional-functions

Thanks for the reference.

what gains are there from having the root be a class?

Consistency with the ecosystem when there is no benefit otherwise. It seems this is being confused at least a little with an assumed superiority of language implementations.

WRT async/await, this is a difficult issue to respond to with likely no clear criterium for an objective assessment. On one hand, cancellability is a potentially superior feature requirement, yet with no current viable implementation. On the other, async/await is a superior drop-in replacement for the existing asynchrony alternatives, despite being only just recently added.

@rlidwka

I should probably write a browser plugin that replaces word idiomatic with my personal preference in this entire thread.

Iit would be inaccurate. This issue is explicitly about functionality and whether exploring whether node-flavored versions provide, not style.

Not really. See "native promises vs bluebird" above

You missed the labeled assumption in the OP.

It is also far cleaner and reader/learner-friendly to write javascripts without semicolons.

This is purely stylistic, and unrelated to node.

I do seriously believe that async generators are superior to promises and callbacks right now.

As do I, but that wasn't my point.

I don't believe anything could be worse than silent failure.

Silent failure exists with callbacks as well and is arguably more common and error prone through manual propagation.

I wish io.js used thunks instead of callbacks, [...] It would be the same idea as promises

Promises provide chainability, caching, automatic error handling and a whole suite of features that callbacks and thunks don't offer.

@bmeck
Copy link
Member

bmeck commented May 11, 2015

@CrabDude

Async/await are a little better, but may be confined to situations that actually set us back from callbacks which is a big red flag to me; I leave my work in userland until they can deal w/ resource cleanup / lifecycle (lockfiles are a classic example).

Also, remember that caching is not always what people want / you need to be more careful to avoid memory leaks.

@chrisdickinson
Copy link

Unfamiliar. Reference?

WHATWG/streams. Also, the snippet in my original comment shows how that operates in practice (you can provide a transform function as part of the options parameter to Transform).

@hax
Copy link

hax commented May 12, 2015

Totally agree!
And one more:

@spion
Copy link

spion commented May 12, 2015

@bmeck have you seen bluebird's resource management features? They can easily be written on top of native promises. Are they not good enough?

@bmeck
Copy link
Member

bmeck commented May 12, 2015

@spion they are technically enough, but they require code an using both .using and .cancel which gets really nightmare levels quickly since inner promises that invoke locks need to be passed the cancel mechanism for the task they are locking.

@CrabDude
Copy link
Author

@bmeck

Why can't that be written with async/await?

async foo(abort) {
  let release = await lockfile('filename.lock', abort);
  try {
     db.query(...);
     fs.write('filename', ...);
     ...
  }
  finally {
     release();
  }
}

@hax Unless I'm mistaken, those both exist within the V8 heap, while Buffer does not. I think reconciling that would be a prerequisite.

@bmeck
Copy link
Member

bmeck commented May 12, 2015

@CrabDude there is no way to abort the result of foo().

  1. promises are not cancellable (don't care to talk about this, too many threads on this, they are still not...)
  2. abort in the current spec would not be available since it is created as a result value for foo(). this is similar to how dynamic properties in es5 cannot be attached prior to an object being created (x={};x[y]=1).
  3. aborting a result gets to an odd case that is still under contention due to how generators and finally work.
function* foo(abort) {
  try {abort();}
  finally {return 'no';}
}

will return 'no'. This behavior is actually really stinking hard to map to Promises and hard to say if it is right or wrong for the async function / has not been explored.

@CrabDude
Copy link
Author

@bmeck Ah. This is very interesting, and as you say "complex". Equivalent functionality could only be accomplished by checking compromised at every call to await, and it's the manual control over function*'s continuation that enables this. You would need either cancel, throw (probably implemented as cancel with error) or some ability to preemptively early-return through functionality like blocks.

@bmeck
Copy link
Member

bmeck commented May 12, 2015

@CrabDude https://github.com/bmeck/generator-runner is what we are using right now, but more work needs to be done wrt. this before going all in and saying it is a good way to do things.

@spion
Copy link

spion commented May 12, 2015

@bmeck bluebird 3.0 will have some interesting new cancellation semantics. Not sure if they'll do what you want.

@petkaantonov
Copy link

Yes 3.0 cancellation works with generator cancellation (.return) although the application code will be pretty ugly currently in v8 since it doesn't implement Generator#Return

Although it wouldn't affect the example since it doesn't have a catch statement, but if you did, you'd have to manually write the boilerplate check at the start of every catch block:

catch (e) {
    if (e === coroutine.returnSentinel) throw e;
}

@q2dg
Copy link

q2dg commented Aug 10, 2015

Sorry for not contributing anything positive to this thread, but I didn't want to miss the chance to say publicly that I TOTALLY agree with @CrabDude

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests