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

Node 6.5 fails when using --harmony-proxies flag #8388

Closed
colbycheeze opened this issue Sep 2, 2016 · 21 comments
Closed

Node 6.5 fails when using --harmony-proxies flag #8388

colbycheeze opened this issue Sep 2, 2016 · 21 comments
Labels
doc Issues and PRs related to the documentations. v8 engine Issues and PRs related to the V8 dependency.

Comments

@colbycheeze
Copy link

$ node --version                  
v6.5.0
$ node --harmony_proxies          
node: bad option: --harmony_proxies
$ nvm use 6.4
Now using node v6.4.0 (npm v3.10.3)
$ node --version        
v6.4.0
$ node --harmony_proxies
> 

I originally found this while doing a Travis build.
https://travis-ci.org/IBM-Bluemix/logistics-wizard-webui/builds/156878342

@cjihrig
Copy link
Contributor

cjihrig commented Sep 2, 2016

v6.5.0 updated V8, so I'm assuming that flag (which is now unnecessary) has been removed there.

@mscdex mscdex added v8 engine Issues and PRs related to the V8 dependency. v6.x labels Sep 2, 2016
@silverwind
Copy link
Contributor

Yep, no such option there. See node --v8-options for valid options.

@silverwind
Copy link
Contributor

silverwind commented Sep 2, 2016

BTW, I get that this is a pain point, but unfortunately we can't really keep v8 options in sync with the semver policy, especially for experimental flags like the --harmony ones.

Reopening for possible documentation update to mention these flags are to be considered experimental.

@colbycheeze
Copy link
Author

Gotcha. I just had to use the flag because a package in my tests was using global.Proxy so it forced me to pass it to run. Is that method built into v8 now?

@silverwind
Copy link
Contributor

Yes, looks like it:

$ node -p Proxy
[Function: Proxy]

@Fishrock123
Copy link
Contributor

Hmm, not good. We should probably parse it as a no-op to maintain 100% backwards compat.

@MylesBorins
Copy link
Contributor

+1 @Fishrock123

I know that in the past we have not treated V8 flags as "semver'y", but we should not be breaking code that was working before.

Add this to the list of things to think about for updated V8 mid release in the future

@colbycheeze
Copy link
Author

@Fishrock123 that would be cool, would allow us to keep using the latest Node in prod without having the annoyance of explaining to everyone doing dev that they need to all update their Node to 6.5 (can just leave in the flag for them, and in prod it would just ignore it right?)

evanlucas added a commit to evanlucas/node that referenced this issue Sep 2, 2016
The upgrade from V8 5.0 to V8 5.1 removed the --harmony-proxies flag.
This restores them as no-ops to prevent breakage.

Fixes: nodejs#8388
@dnalborczyk
Copy link
Contributor

I would advise against taking any action.

The flags were always advertised as a playground to try and test new features - explicitly not production ready! Anything can break during v8 staging, features might change or they might get scratched all together from the ECMA spec - following the removal from v8 and eventually Node itself.

https://nodejs.org/en/docs/es6/

Please note that these are incomplete and possibly broken features of V8, so use them at your own risk:

@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 3, 2016

I would say that it makes no sense to add these flags on master. whether
or not it is officially supported, it is a behavior change on v6. floating
a patch on v8 of node 6 seems reasonable to me. it is a one time thing with
minimal overhead.

/cc @nodejs/v8 for their thoughts

On Fri, Sep 2, 2016, 9:14 PM dnalborczyk notifications@github.com wrote:

I would advise against taking any action.

The flags were always advertised as a playground to try and test new
features - explicitly not production ready! Anything can break during
v8 staging, features might change or they might get scratched all together
from the ECMA spec - following the removal from v8 and eventually Node
itself.

https://nodejs.org/en/docs/es6/

Please note that these are incomplete and possibly broken features of V8,
so use them at your own risk:


You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#8388 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV6nzPEosUDRHGb2pJT0NGDXxjPvkks5qmMoRgaJpZM4Jz3Rq
.

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Sep 3, 2016

I would say that it makes no sense to have these flags on
master.

@thealphanerd Do you want to disallow any flags in master? That would be very unfortunate.

@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 3, 2016

@dnalborczyk I've edited my comment above

I meant that it doesn't make sense to add these specific flags to master

edit: updated v8 on v6.5.0 resulted in the flags being removed. I am suggesting that we should add no-ops for those flags to avoid breaking behavior, but to not apply that patch to master.

@dnalborczyk
Copy link
Contributor

Gotcha. I can totally see your reasoning behind it, though it feels like a waste of resources. If proxies were to have the slightest change in behavior it could smoke up the app nonetheless (with or without no-ops).

I also want to remind that this kind of thing can (and likely will) happen over and over again.

Just my 2 cents.

@MylesBorins
Copy link
Contributor

@dnalborczyk this is the first time we have ever bumped a version of V8 in an already released branch and it brought up a whole bunch of interested / unexpected edge cases. Fixing this would likely take less than an hour of total effort. I imagine that it would likely happen less than twice a year, even less if we consider this is really only going to happen on LTS release lines. An hour or two a year seems pretty reasonable to stop people's stuff from breaking... especially considering that things are arguable more stable when the flag has been dropped.

@silverwind
Copy link
Contributor

One concern with making the flag graceful is that we're making the impression that 6.4.0's experminal Proxy with the flag is 100% compatible with 6.5.0's Proxy without it. Do we know for sure?

@targos
Copy link
Member

targos commented Sep 3, 2016

There is a slight misunderstanding here. V8 shipped proxies without a flag in 4.9: http://v8project.blogspot.ch/2016/01/v8-release-49.html
Using the flag in v6.x has always been a no-op and the feature was not considered experimental anymore. It was only here to possibly opt out with --no-harmony-proxies in case of an unexpected issue with the feature.
The flag was removed in V8 5.1 beacause nothing bad happened during the lifecycle of two Chrome versions.

@ofrobots
Copy link
Contributor

ofrobots commented Sep 3, 2016

--harmony flags are an experimental feature. By using experimental features, e.g. AsyncWrap, harmony etc., you opt into the risk that backwards incompatible changes can happen.

Still, removal of this flag was not an intentional change in 6.5.0. It is something that slipped through the cracks as part of the V8 update. I'd be in favor of adding back a nop flag if someone wants to put together a PR.

EDIT: grammar.

@Fishrock123
Copy link
Contributor

--harmony flags are an experimental feature. By using experimental features, e.g. AsyncWrap, harmony etc., you opt into the risk that backwards incompatible changes can happen.

I do agree that this is very much true.

@thetutlage
Copy link

We should maintain the backward compatibility, since projects are written for multiple versions of Node.js. It is a pain for me to run CI tests for multiple versions too.

@ofrobots
Copy link
Contributor

ofrobots commented Sep 6, 2016

I will try to put together a patch adding back the removed harmony flags (probably tomorrow).

ofrobots added a commit to ofrobots/node that referenced this issue Sep 13, 2016
Add back the no-op harmony shipping flags that were removed in V8 5.1
to increase compatibility with V8 5.0 that we had been shipping before
v6.5.0. These flags do nothing.

Fixes: nodejs#8388
Ref: nodejs#8395
PR-URL: nodejs#8445
Reviewed-By: addaleax - Anna Henningsen <anna@addaleax.net>
Reviewed-By: thealphanerd - Myles Borins <myles.borins@gmail.com>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: evanlucas - Evan Lucas <evanlucas@me.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@ofrobots
Copy link
Contributor

Fixed by #8445.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests