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.

Sign up for GitHub

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

Jump to bottom

harmony-regexp-properties should be not enabled without a flag yet #6251

Closed
ChALkeR opened this issue Apr 17, 2016 · 10 comments · Fixed by #6290
Closed

harmony-regexp-properties should be not enabled without a flag yet #6251

ChALkeR opened this issue Apr 17, 2016 · 10 comments · Fixed by #6290
Labels
v8 engine Issues and PRs related to the V8 dependency.
Milestone
6.0.0

Comments

@ChALkeR
Copy link
Member

ChALkeR commented Apr 17, 2016 •
edited
Loading

Extracted from #6155 (comment)
This is a v8 issue, upstream report: https://bugs.chromium.org/p/v8/issues/detail?id=4926

This affects v6 RC 2.

/cc @vsemozhetbyt, @jasnell, @ofrobots

I think this is worth a separate issue and that we should keep an eye on it — releasing 6.0.0 with this issue would mean that any future fix to this (i.e. hiding harmony-regexp-properties behind a flag) would technically be a semver-major change.
@ChALkeR ChALkeR added the v8 engine Issues and PRs related to the V8 dependency. label Apr 17, 2016
@ofrobots
Copy link
Contributor

@ChALkeR Thanks for extracting this into a separate issue. I would expect that a fix for this is likely to be merged back to V8 5.0 soon. Even if it wasn't, I think you're using too strict a definition of semver-major. Every bug fix, by definition, an is observable change. If a bug lives in the ecosystem for a very long time, and people start depending on that specific behaviour, only then it comes a semver major.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Changes to command line flags exposed by node are always semver-major but I think this one would be a gray area -- I'm not sure we need to treat flags exposed by v8 as a major.

@ofrobots
Copy link
Contributor

@jasnell The command line flag already exists; the bug is that it wasn't being respected in code.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Awesome, then I'd definitely be surprised if it was anything more than a minor.

@ChALkeR
Copy link
Member Author

ChALkeR commented Apr 18, 2016 •
edited
Loading

@jasnell The issue here is that the code works without the harmony flag, when it shouldn't.

The fix would make code that currently works out of the box not working anymore without a flag, that looks semver-major to me.

@bnoordhuis
Copy link
Member

FWIW, https://bugs.chromium.org/p/v8/issues/detail?id=4743 is the tracking issue. v8/v8@e0d0c96a is the commit that introduced the flag but v8/v8@aba76874 first started using it. The former is in master, the latter is not; there are a few intermediate commits as well.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

@ChALkeR ... understood, but even then I'm not 100% sure it rises to a semver-major. In any case, let's try to get it fixed before v6 is cut ;-)

@ChALkeR ChALkeR added this to the 6.0.0 milestone Apr 18, 2016
@ChALkeR
Copy link
Member Author

ChALkeR commented Apr 19, 2016 •
edited
Loading

@jasnell Hm. Imo one way how this would be not a semver-major is this issue being explicitly documented as a bug in the release notes. That would solve it, perhaps.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

+1, I prefer that approach

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Apr 19, 2016 •
edited
Loading

Fixed in the V8 5.0.71.33: https://bugs.chromium.org/p/v8/issues/detail?id=4926#c4

@ofrobots ofrobots mentioned this issue Apr 20, 2016
Merged
2 tasks
ofrobots added a commit to ofrobots/node that referenced this issue Apr 20, 2016
@ofrobots
deps: upgrade to V8 5.0.71.33
4fbd375 
This picks up the fix for harmony-regexp-properties being enabled
without a flag.

V8-Commit: v8/v8@27ac008
Fixes: nodejs#6251
PR-URL: nodejs#6290
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ChALkeR - Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
@ofrobots @joelostrowski
deps: upgrade to V8 5.0.71.33
3b337cb 
This picks up the fix for harmony-regexp-properties being enabled
without a flag.

V8-Commit: v8/v8@27ac008
Fixes: nodejs#6251
PR-URL: nodejs#6290
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ChALkeR - Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
@ofrobots @jasnell
deps: upgrade to V8 5.0.71.33
809ed52 
This picks up the fix for harmony-regexp-properties being enabled
without a flag.

V8-Commit: v8/v8@27ac008
Fixes: #6251
PR-URL: #6290
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ChALkeR - Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: indutny - Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: targos - Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: jbergstroem - Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: cjihrig - Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
6.0.0
Development

Successfully merging a pull request may close this issue.

deps: upgrade to V8 5.0.71.33 ofrobots/node
5 participants
@ofrobots @bnoordhuis @ChALkeR @jasnell @vsemozhetbyt