-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
harmony-regexp-properties should be not enabled without a flag yet #6251
Comments
@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. |
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. |
@jasnell The command line flag already exists; the bug is that it wasn't being respected in code. |
Awesome, then I'd definitely be surprised if it was anything more than a minor. |
@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. |
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. |
@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 ;-) |
@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. |
+1, I prefer that approach |
Fixed in the V8 5.0.71.33: https://bugs.chromium.org/p/v8/issues/detail?id=4926#c4 |
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>
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>
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>
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.
The text was updated successfully, but these errors were encountered: