-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Destructuring of arrow function arguments via computed property throws in v6.x #10347
Comments
According to node.green it should work in 6.9.0–6.9.2. This example from "computed properties" tooltip there works fine in the 6.9.2 function test(){
var qux = "corge";
return function({ [qux]: grault }) {
return grault === "garply";
}({ corge: "garply" });
}
console.log(test()); |
@vsemozhetbyt example from node.green indeed works. But the same example, slightly changed, throws: const qux = "corge";
const fn = ({ [qux]: grault }) => {
return grault === "garply";
};
console.log(fn({ corge: "garply" })); UPDATE: What is interesting - if replace arrow function with the regular function - it works. |
So this is corrected formulation: destructuring of arrow function arguments via computed property throws in the Node.js 6.9.2: // does not throw in the Node.js 6.9.2
const obj = { key: 'value' };
const keyComputed = 'key';
function fn({ [keyComputed]: param }) {}
fn(obj); // throws in the Node.js 6.9.2
const obj = { key: 'value' };
const keyComputed = 'key';
const fn = ({ [keyComputed]: param }) => {};
fn(obj); |
Thanks @vsemozhetbyt, I've updated the issue title and description. |
It seems the bug is actual only for destructuring. Other evaluations in the parameters scope are OK. // does not throw in the Node.js 6.9.2
const obj = { key: 'value' };
const keyComputed = 'key';
function fn(param = obj[keyComputed]) { console.log(param); }
fn(); // does not throw in the Node.js 6.9.2
const obj = { key: 'value' };
const keyComputed = 'key';
const fn = (param = obj[keyComputed]) => { console.log(param); };
fn(); |
I see the test case for this in V8 on 6.x branch: https://github.com/nodejs/node/blob/v6.x/deps/v8/test/mjsunit/es6/destructuring.js#L893 Should not it fail? |
Well, this throws: // throws in the Node.js 6.9.2
var y = 'a';
var g20 = ({[y]: x}) => { var y = 'b'; return x; };
require('assert').equal(1, g20({a: 1, b: 2})); I don't know why it does not fail there( |
Maybe these tests are not actually tested during Node.js build (unlike Node tests)? |
the V8 tests are not run during the normal node CI
…On Mon, Dec 19, 2016, 7:50 PM Vse Mozhet Byt ***@***.***> wrote:
Maybe these tests are not actually tested during Node.js build (unlike
Node tests <https://github.com/nodejs/node/tree/master/test>)?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#10347 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAecV_qmc70Sg0sn2EgYzPqz3CeYCqAhks5rJyZdgaJpZM4LRRZ2>
.
|
This seems to work correctly on the newest V8 (version 5.7). |
@hashseed I think this is a fix we want to backport. Do I read it correctly that it was broken because of some interaction with Node (because V8 has a test for it and it's not a regression test)? |
@fhinkel I'm under the impression that it's just broken due to V8, and should be reproducible on d8 at an older version. I haven't tested though. |
Just compared node with d8 (V8 5.1.281.89) both built from (function(){
const qux = "corge";
const fn = ({ [qux]: grault }) => {
return grault === "garply";
};
var _print = typeof console === 'undefined' ? print : console.log;
_print(fn({ corge: "garply" }));
})(); Test fails with both, only when the code is wrapped in an IIFE (simulating our module wrapper). |
The bug is already fixed in
|
Looks like a gclient sync issue. |
Still happens after removing binutils and syncing again. |
Original commit message: Rewrite scopes in computed properties in destructured parameters While we properly handled scopes of initializers in destructured parameters, we never did the right thing for computed properties. This patch fixes that by factoring out PatternRewriter's scope rewriting logic and calls it for the computed property case. BUG=chromium:620119 Review-Url: https://codereview.chromium.org/2084103002 Cr-Commit-Position: refs/heads/master@{nodejs#37228} Fixes: nodejs#10347
Original commit message: Rewrite scopes in computed properties in destructured parameters While we properly handled scopes of initializers in destructured parameters, we never did the right thing for computed properties. This patch fixes that by factoring out PatternRewriter's scope rewriting logic and calls it for the computed property case. BUG=chromium:620119 Review-Url: https://codereview.chromium.org/2084103002 Cr-Commit-Position: refs/heads/master@{#37228} Fixes: #10347 PR-URL: #10386 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Looks like @targos fixed it. |
Thanks. I forgot that we can only close issues by pushing on master! |
The test case is taken from: https://github.com/nodejs/node/blob/v6.x/deps/v8/test/mjsunit/es6/destructuring.js#L893
The text was updated successfully, but these errors were encountered: