-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
v8: let Object.defineProperty work with process.env #2999
Conversation
For posterity, that CL landed in v8/v8@fa3e6c0 and doesn't touch src/runtime/runtime-object.cc. I'm a bit hesitant to land this PR. The changes to runtime-object.cc don't look obviously correct to me. /cc @nodejs/build - can we have an option to run the V8 test suite on the CI as well? |
Is this related to #3375? cc @trevnorris? |
@Fishrock123 Don't believe so. |
The question is: will there be another 3.3.x release? |
Last 3.x release scheduled @ #3465, if someone really wants this in then you'll have to work hard at it. If @bnoordhuis is not happy with the look of it then I'd not be hopeful. /cc @nodejs/v8 - anyone brave enough to sign off on this? |
There will be no new 3.x releases, per #3465 (comment). |
This fixes #2998.
This is only a back port to v3.x since the bug will not emerge in v4.x. It turned out that the v8 team encountered a similar bug regarding
window.localStorage
in Chrome and landed a patch in Jun which is consequently applied into Node 4.x.https://codereview.chromium.org/1180073002
However the patch is too big to back port to v3.x. I made a minimal version and therefore come this PR.