src: enable --harmony by default #6999

Closed
wants to merge 2 commits into
from

Projects

None yet
@bnoordhuis
Member

Harmony support is probably the most anticipated feature in v0.12.
Enable it by default so module authors can safely assume that Harmony
features like generators and proxies are available. It's possible to
disable Harmony features again with the --no-harmony flag.

Update src/node_contextify.cc to make it stop copying properties from
the parent context indiscriminately. It copies over the Proxy object
from the parent context but the new context has one of its own.

Update test/simple/test-repl.js; it had a test that checks for something
that's prohibited in ES5 strict mode but allowed in ES6, namely block
functions.

Suggested reviewers: I don't know, @indutny, @tjfontaine and @trevnorris? I'm nothing if not inclusive.

bnoordhuis added some commits Jan 30, 2014
@bnoordhuis bnoordhuis test: don't compute knownGlobals lazily
Conditional globals like 'gc' should only be recognized when --expose_gc
is set.  The global.gc feature check works only when done eagerly, else
it lets through a leaked variable called 'gc'.
e41896b
@bnoordhuis bnoordhuis src: enable --harmony by default
Harmony support is probably the most anticipated feature in v0.12.
Enable it by default so module authors can safely assume that Harmony
features like generators and proxies are available.  It's possible to
disable Harmony features again with the --no-harmony flag.

Update src/node_contextify.cc to make it stop copying properties from
the parent context indiscriminately.  It copies over the Proxy object
from the parent context but the new context has one of its own.

Update test/simple/test-repl.js; it had a test that checks for something
that's prohibited in ES5 strict mode but allowed in ES6, namely block
functions.
707fc40
@Nodejs-Jenkins

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

The following commiters were not found in the CLA:

  • Ben Noordhuis

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@indutny
Member
indutny commented Jan 30, 2014

It is not yet enabled by default in v8, I don't see any reason except user expectations.

@reqshark reqshark referenced this pull request in expressjs/express Jan 30, 2014
Closed

tests for req/res proto restoration #1114

@othiym23

I don't think it's appropriate to turn on ES6 in Node by default until V8 has had a chance to get their implementation in line with the standard, which is still a few months away from being complete. Just off the top of my head, I know a bunch of stuff is in flight for module linking and loading, and there are some non-compliant aspects to V8's promise implementation. @domenic, what do you think?

The other updates are valuable, and should probably be landed in a separate PR.

@domenic
Member
domenic commented Jan 30, 2014

I agree that V8's ES6 implementation is generally pretty bad and non standards-complaint; we should not turn it on. Maybe individual low-risk features, like octal literals, but e.g. their promises and proxy implementations are both spectacularly out-of-line with the spec.

I don't understand the contextify change or what it accomplishes.

@indutny
Member
indutny commented Jan 30, 2014

Ok, considering your comments, I'm now sure that we should not do it just yet. Let's wait for v8 team to enable it by default. I think that as soon as their implementation will be stable and compatible with spec - they will do it.

@indutny indutny closed this Jan 30, 2014
@tjfontaine

I'm going to reopen this for now, for further discussion.

The official Node opinion thus far has been to not enable these features before v8 enables them by default. There are arguments for why the v8 team itself hasn't enabled them and by and large they probably apply to Node as well. That being said, we could probably perform an audit query over the current npm packages to get a frame of reference of the impact of at the least keyword conflicts (it won't be a perfect query but it could give us at least some insight).

That being said, I think there's a few options we could consider as a compromise.

My gut reaction was to do this as a basename() check, (I threw out--even though they made me chuckle--nodeharm and noduh) so if you invoke node as node_h you would get --harmony by default. From a deployment mechanism this is relatively easy for us to ship, it would just be a sym/hard link, but it could also be done with npm install -g node_h that handles the hard/soft link for you. When compared to running with --harmony manually (or conversely --no-harmony) this simplifies the logic for child_process.spawn(process.execPath). The problem, as it exists today, is you have to do a mixture of process.execPath, process.execArgv, and process.argv parsing to get that right. With the basename model, there's a reasonable assurance that your child process will end up including harmony.

Arguments against this include it being a fairly obtuse mechanism and an anti-pattern, as well as if it were Node actively installing this link it's one more thing to maintain in perpetuity because someone would be writing scripts as #!/usr/bin/env node_h. It's also unclear what the path forward would be on Windows, that is if there's a sane link mechanism that could be used, or if it would require a separate binary. In the case of people deploying this as a copy of the binary you run the risk of a version mismatch, which would be subtle and frustrating to debug for many users.

Another potential compromise, is the concept of a more surgical approach. Enabling only those flags which we deem to be appropriate for every day use. It's easier to describe the scenarios where we wouldn't enable a harmony feature by default.

  • Incomplete specification, which means the implementation in v8 would be subject to change
  • A non-conformant implementation in v8
  • An unacceptable risk of breaking existing code
  • Unacceptable performance implications
    • presumably this would imply a performance implication across Node as a whole, not just merely about the performance of the feature (though that could be as well)

The list of potential v8 harmony options, along with @domenic's disposition on if they should or should not be enabled:

  • --harmony_typeof (enable harmony semantics for typeof)
    • -1 (shouldn't exist anymore)
  • --harmony_scoping (enable harmony block scoping)
    • +1
  • --harmony_modules (enable harmony modules (implies block scoping))
    • -1
  • --harmony_symbols (enable harmony symbols (a.k.a. private names))
    • +1
  • --harmony_proxies (enable harmony proxies)
    • -1
  • --harmony_collections (enable harmony collections (sets, maps, and weak maps))
    • +1 (only a subset has been implemented)
  • --harmony_observation (enable harmony object observation (implies harmony collections)
    • -1
  • --harmony_generators (enable harmony generators)
  • --harmony_iteration (enable harmony iteration (for-of))
    • -1
  • --harmony_numeric_literals (enable harmony numeric literals (0o77, 0b11))
    • +1
  • --harmony_strings (enable harmony string)
    • +1
  • --harmony_arrays (enable harmony arrays)
    • +1
  • --harmony_maths (enable harmony math functions)
    • +1

Other considerations are that the unstable branch currently has v8 3.22, 3.23 is the current stable and I'm open to updating to that before the 0.12 release, but 3.24 is the current development branch I am not sure I would be comfortable in releasing 0.12 with it, without giving it more time to bake.

@tjfontaine tjfontaine reopened this Jan 30, 2014
@othiym23

I'm uncomfortable with rolling ES6 support out piecemeal. As a tooling engineer, this creates a feature matrix for support that just seems nightmarishly complex. Each additional language feature adds in more conditional logic to the various bits of my codebases that need to accomodate specific features. Also, because there's no rigid correlation between the (implicit) version of JavaScript implemented by a given version of V8, the version of V8 in the runtime, and the version of Node, this means that new features are going to be trickling out over time, which implies the possibility that there will be more tests necessary for specific point releases of Node to deal with changes in the underlying language. If the versioning were a little more lockstep, I'd probably be more comfortable with this.

In general, I'd be more comfortable waiting until V8 is confident enough in their implementation of the standard to drop the flag than opting the community in gradually.

@domenic
Member
domenic commented Jan 30, 2014

I can see @othiym23's point of view. Counterarguments:

  • Turning on known-safe features helps avoid people turning on and using --harmony itself, which would opt them into known-unsafe features (like proxies or similar).
  • It's pretty unlikely all of ES6 will be implemented in V8 at any point before, say, 2018. (Judging from time to ES5 implementation from spec completion, plus a multiplier for the relative complexity of ES6 vs. ES5.) More likely, they will wait for the spec to be pronounced final, then turn on all the features they've completed, which will still be a piecemeal selection.
@Fishrock123
Member

I'm pretty certain we are going to see both lots of production code with these flags & lots of projects / libs requiring these flags, even if not enabled by default.

👍 to @domenic's counterarguments.

@othiym23

I'm pretty certain we are going to see both lots of production code with these flags & lots of projects / libs requiring these flags, even if not enabled by default.

I'm fine with this, as that allows vendors to choose what they do and don't / can and can't support. New Relic is all about a seamless deployment process and high-quality support, and to do a good job of this, we need to be able to focus where we spend our resources. We have to support everything that's enabled out of the box in Node core -- everything else needs a justification. I'm sure we're not alone in this respect. People building platforms on top of Node are strongly affected by this decision.

Turning on known-safe features helps avoid people turning on and using --harmony itself, which would opt them into known-unsafe features (like proxies or similar).

I don't find this argument very compelling. V8 gives you more fine-grained control over which features you enable via the various --harmony_ switches, and Node users have demonstrated very clearly that they'll do whatever they feel like, regardless of whether it's a good idea.

It's pretty unlikely all of ES6 will be implemented in V8 at any point before, say, 2018. (Judging from time to ES5 implementation from spec completion, plus a multiplier for the relative complexity of ES6 vs. ES5.) More likely, they will wait for the spec to be pronounced final, then turn on all the features they've completed, which will still be a piecemeal selection.

My hope is that implementors are enough on top of things this time around that adoption will be significantly faster for ES6 than for ES5, but you're right that the complexity of ES6 makes this seem like wishful thinking on my part. Either way, the way Node just drops whatever the latest stable build of V8 is into point release of Node also makes me nervous, it just hasn't been much of an issue until very recently because it's very rare that there are breaking semantic changes to the language V8 implements itself. I would welcome Node being more deliberate / conservative about adopting new versions of V8.

@jonathanong

from a module maintainer's perspective (https://github.com/koajs/koa), i would very much like --harmony-generators to be enabled by default. it would be easier to tell people to use 0.12+ vs. 0.11.3+ with --harmony-generators. i actually don't like telling people to use --harmony at all. it would also make deployment easier as some PaaS' don't allow you to pass these flags (they might now, but before most didn't).

also, i'm assuming node doesn't change v8 versions drastically within stable versions (especially now with those annoying API changes). i don't want to wait until 1.0.0 to not use --harmony-generators because that could be a very long time.

@trevnorris

While I personally dislike much of ES6, I'll try to put that aside.

First, if we are going to enable ES6 features before they're officially
supported then I'd like to see accompanying tests that are written not from
the current v8 implementation but from the specification itself.

Second, I want users to understand that - apart from simple things like
the additional String APIs - ES6 isn't getting into core. Native support
for generators, etc., will only bloat what I consider to be getting a
really thick code base. Instead of more fluffy abstraction from what's
going on under the hood, it needs to be exposed more simply for module
developers to implement their interface of choice.

@trevnorris

I should specify better about the tests. I'm sure v8 already supplies
those, and I want to make sure their tests conform to the specification and
pass.

@Rush
Rush commented Jan 31, 2014

One good reason why not to enable harmony collections. If other features have the same "quality" then better not. Run the following with node --harmony.

> (new Set([1, 2, 3])).size
0          <--- should be 3, also works in Firefox and spec says it should work
@domenic
Member
domenic commented Jan 31, 2014

Yes, the Set constructor is not implemented. That's one of the things that's left out of the implemented subset of the collections stuff.

@Rush
Rush commented Jan 31, 2014

A friend of mine started doing some things in Node.JS and he said he really really wants to write his code "semantically correctly" as in "using correct container for the job". After trying out node --harmony and this Set contructor he basically stated that JavaScript is broken and without hope. It took me some time to explain that he should not take unstable features for granted and that is why unstable features such as harmony should not be enabled by default. Set is probably one of the "hottest" wanted features and imagine how many bugs could be triggered by later fixing the previously broken behavior if Set were to be introduced by default in its current state. I guess it is not possible to introduce these new features in a module scope only?

@mikeal
Member
mikeal commented Jan 31, 2014

Adding the features piecemeal causes a compatibility nightmare similar to the Python 2.x releases. As different features stabilize you enable them which means that all modules using those features only work with that release forward. These changes are higher impact than any of the changes we've seen in core since 0.8 and make compatibility between releases a much more complicated story.

IMO it would be much better to say "this is the ES6 release" and enable everything when it's ready. That would mean only one forward compatibility break for the module ecosystem to deal with and only one node version people who are using these features have to worry about hitting when they see ES6 features being used in a module.

@mikeal
Member
mikeal commented Jan 31, 2014

@domenic I have the exact opposite perspective on people running with --harmony :)

Only crazy people are running that in production. Which is good, we need crazy people, and we need people who are living on the edge and experimenting, that's why the features are available at all behind a flag.

For the people who are experimenting it is actually beneficial for the community for them to be opting in to more unstable features because that's the only avenue we have for testing, early usage, and feedback.

Core's primary concern should be maintaining stability for the majority of the module ecosystem and production users and not stabilizing the people who are living on the edge and helping us everyday by testing unstable features :)

@domenic
Member
domenic commented Jan 31, 2014

Note that unlike Python 2/3, ES5/6 is backward compatible in every way. (No ES5 code will break by turning on ES6 features.)

In general, I would encourage moving away from a version-number based way of thinking. Much like every other web spec, ES is evolving toward a model that does not consist of rigidly-defined versions, but instead modular features being added periodically, with interplay between the spec and engines informing evolution. The version number is not very meaningful. For example V8 will likely ship Object.observe (ES7) before direct proxies (ES6).

Now. If you want to abdicate responsibility for deciding when something is "ready" to V8, i.e. wait until they turn on the flag by default, that's a fine and consistent approach. But we shouldn't pretend they're going to do an "ES6" version suddenly. It's going to be just as piecemeal as what we are contemplating here.

@mjsalinger

New ECMAScript versions tend to get implemented over a very long stretch of time - if we decide to wait until v8 implements the entire spec, we may be waiting for years for some of its benefits. I also don't think, however, that providing a whole bunch of piecemeal harmony flags is a good idea either.

The node team should make the decision on what harmony features can be considered "stable" as of v8 v 3.23, and implement those into node core. Other non-stable features can continue to be accessed by the --harmony flag. Additional harmony features can be added as they become more stabilized, but only in the next major version (i.e. freeze the harmony features that are added in 0.12, and then no new harmony features can be added until 1.0, then 1.2, etc...)

@isaacs
isaacs commented Jan 31, 2014

Note that unlike Python 2/3, ES5/6 is backward compatible in every way. (No ES5 code will break by turning on ES6 features.)

This is, in theory, true. However, if we enable (say) generators and block functions, but not collections and modules, etc, then you have a state where your module half-works in some versions of Node, but not others.

If we enable --harmony then that's effectively what we're doing. For me, it's less an issue that these features break ES5, as much as that they are broken, and not functioning as intended. It seems less harmful to require opt-in for a feature that doesn't work, than to turn it on, and have to remember "Oh, yeah, 0.12 had that weird bug, promises existed but they were broken."

If I'm just out of the loop, and we can rely on the harmony stuff today, then I agree with @bnoordhuis and @mikeal that it'd be better to just say "Harmony starts at 0.12", and be done with it. Maybe the "broken in --harmony" issue is not that big a deal. (After all, there are ways in which vm is broken in 0.10, and we fix it in 0.12, and that's fine.)

Adding an additional symlink, and a check on argv[0] for this, seems like a litterbug approach to a temporary problem. Presumably, eventually --harmony features will all be good, and node_h will still have to be kept around to support all the modules doing #!/usr/bin/env node_h.

We should either bite this bullet now, and just turn on --harmony for everyone, or decide it's too tough a bullet to bite now, and bite it at some future date instead. Personally, I am leaning more towards doing it now rather than later.

Aside: if Proxies are available, I believe we can move most of node_contextify.cc into JS.

@othiym23

@trevnorris Work is underway to get test262 up to speed with ES6. Seeing where V8 --harmony stands with respect to it is on my list of things to do. I haven't looked too closely at its licensing, but it may be compatible enough to include as a dependency once it's ready to test ES6 conformance.

I think a lot of people share your reservations about ES6 (me included!), but it is coming, and a lot of people want the features it offers. I also know that it's not just generators or collections that people want -- there's a lively discussion going on elsewhere on how to make Node's module system interoperable with ES6 modules. Some of the more prominent people in that discussion seem to think Node might make the switch!

@isaacs
isaacs commented Jan 31, 2014

@domenic I think @mikeal was referring to the 2.x release train, where new language features were added gradually, rather than the 2 -> 3 apocalypse.

@Rush
Rush commented Jan 31, 2014

It may be notable that Firefox is already allowing using "let" to declare variables and allows using Set/Map and other ES6 features and it did not cause the end of the world for them.

@othiym23

@isaacs @domenic When the V8 team feels that something is ready enough that it no longer needs to behind the flag, it's appropriate for it to be merged in.

I also think the fragmentation problem can be addressed by linking stable major versions of node (which in current terms means 0.10, 0.12, 1.0 and up) to stable minor versions of V8 (3.22, 3.23, 3.24). If V8 starts dribbling out bits of ES6 in, say, patch releases of 3.24, there's not a lot we can do about that if we want to avoid the pain of maintaining a floating patch to keep up to date with security and stability changes. But that hasn't been the pattern they've been following to this point.

In general, the closer we get to 1.0, the less sense it makes to me to just keep dropping the latest stable versions of V8 into the deps folder, especially as the language implemented by it starts to change.

@domenic
Member
domenic commented Jan 31, 2014

If we enable --harmony then that's effectively what we're doing. For me, it's less an issue that these features break ES5, as much as that they are broken, and not functioning as intended. It seems less harmful to require opt-in for a feature that doesn't work, than to turn it on, and have to remember "Oh, yeah, 0.12 had that weird bug, promises existed but they were broken."

Yes, definitely, this is the key issue. That was the motivation behind the conversation with @tjfontaine in IRC that resulted in the list about "@domenic's disposition on if they should or should not be enabled" above. That is, that list contains features that, given what I have seen, are up to spec (or in the case of collections, the implemented subset is up to spec) and should not change in future releases in a way that would cause the pain you're talking about. For example we should not turn on --harmony_proxies because it gives a very old out of date implementation. Whereas --harmony_numeric_literals is pretty darn baked.

Now, whether that evaluation is entirely rigorous is another story, and feeds in to the questions about test-262 that @trevnorris alludes to and @othiym23 discusses.

@mikeal
Member
mikeal commented Jan 31, 2014

Note that unlike Python 2/3, ES5/6 is backward compatible in every way.

I specifically meant 2.x releases and the forward compatibility breaks between them. All were backwards compatible.

@mikeal
Member
mikeal commented Jan 31, 2014

@RushPL Firefox and spidermonkey have been shipping features that aren't spec complete for years in order to test out these features. The fact that they shipped them should not be an indication of their readiness or spec completeness (while in some cases features may be).

@mikeal
Member
mikeal commented Jan 31, 2014

Quick question: If we did turn on --harmony now could we expect that v8 won't add features that are likely to change or be broken in to the flag?

Even if we can live with what is broken there now it would be nice to have some assurances that it won't actually get worse in the future.

@domenic
Member
domenic commented Jan 31, 2014

Quick question: If we did turn on --harmony now could we expect that v8 won't add features that are likely to change or be broken in to the flag?

I would severely doubt that. I guess maybe the fact that we turned it on might force their hand into creating a new flag, --really-harmony, and leaving --harmony to indicate the existing feature set. (Note that "Harmony" means post-ES4-disharmony, i.e. ES5 and onwards, including 6, 7, ...; it does not mean ES6.)

I would strongly oppose turning on --harmony wholesale because of the broken features (like proxies) that it contains currently. That would be the worst of all possible worlds IMO.

@tjfontaine

Thanks everyone for contributing to the issue, we won't be making this change right now. We can revisit this issue post 0.12, but for now it's too late in the release cycle to introduce such a controversial change.

In the meantime Node will continue to let v8 make the decisions for when features are ready to be enabled.

@tjfontaine tjfontaine closed this Jan 31, 2014
@rwaldron rwaldron commented on the diff Jan 31, 2014
test/common.js
- if (global.ArrayBuffer) {
- knownGlobals.push(ArrayBuffer);
- knownGlobals.push(Int8Array);
- knownGlobals.push(Uint8Array);
- knownGlobals.push(Uint8ClampedArray);
- knownGlobals.push(Int16Array);
- knownGlobals.push(Uint16Array);
- knownGlobals.push(Int32Array);
- knownGlobals.push(Uint32Array);
- knownGlobals.push(Float32Array);
- knownGlobals.push(Float64Array);
- knownGlobals.push(DataView);
- }
+// Harmony features.
+if (global.Proxy) {
+ knownGlobals.push(Proxy);
@rwaldron
rwaldron Jan 31, 2014

Just to be clear, this Proxy is an implementation based on the obsolete design. Literally no one should use this for any reason, ever.

Probably better to lead with WeakMap, which is implemented correctly. This reads like an accidental endorsement of use.

@bnoordhuis bnoordhuis referenced this pull request in nodejs/node Dec 5, 2014
Closed

Proposal: Harmony enabled binary, packages. #90

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment