build: disable harmony classes #272
Conversation
|
LGTM |
| v8.setFlagsFromString('--harmony_classes'); | ||
| eval('"use strict"; class C {}'); // Should not throw. | ||
| eval('"use strict"; let x = 42'); // Should not throw. | ||
| eval('"use strict"; const y = 42'); // Should not throw. |
There was a problem hiding this comment.
afaik let and const are included under another patch, is there a really good reason to have them here? (Also, I thought those worked without strict?)
There was a problem hiding this comment.
--harmony_classes implies --harmony_scoping. The check is to ensure that the the latter is still on. I believe let/const in sloppy mode is still under development.
|
I've added some comments to clarify the --harmony_classes/--harmony_scoping relationship and added another check. Can I have one more LGTM? |
|
--harmony_object_literals behavior will be changing too. That should be disabled. |
|
@domenic What's the change and where was it announced? I didn't see anything on v8-dev. |
|
harmony_object_literals is part of the classes implementation, which is why there's a dependency and why V8 unstaged them all together. In particular, there are changes to super(), toMethod, and the resulting changes for concise methods in object literals. (There's also some non-public (ugh) discussion of an unrelated bug in the concise method implementation.) |
|
In general, the fact that V8 unshipped both classes and object literal extensions in v8/v8@a417b41 should be taken as a loss of confidence in their implementation and I'd encourage iojs to follow those kinds of signals. |
bnoordhuis
force-pushed
the
disable-es6-classes
branch
from
3af4281
to
b53b328
Compare
bnoordhuis
force-pushed
the
disable-es6-classes
branch
from
b53b328
to
0e1850a
Compare
|
Okay, I added a second commit that disables object literals. @domenic et al, PTAL. |
|
LGTM, although as discussed in #251 my preferred approach would be to ship V8 3.30.33.14. |
The V8 development branch has unshipped ES6 classes pending resolution of a number of inheritance edge cases. Disable classes in io.js for the sake of feature parity. See nodejs#251 for background and discussion. PR-URL: nodejs#272 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Domenic Denicola <domenic@domenicdenicola.com>
Per the discussion in nodejs#272, upstream V8 has disabled Harmony object literals for the time being. Do the same for feature parity. PR-URL: nodejs#272 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Domenic Denicola <domenic@domenicdenicola.com>
|
@domenic Noted. I'll land this and I'll bring it up at the go/no go release meeting. Reverting the V8 upgrade is the work of minutes. |
bnoordhuis
force-pushed
the
disable-es6-classes
branch
from
0e1850a
to
4e58211
Compare
The V8 development branch has unshipped ES6 classes pending resolution
of a number of inheritance edge cases. Disable classes in io.js for
the sake of feature parity.
See #251 for background and
discussion.
R=@iojs/tc