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

Don't hoist template literal keys in object-rest-spread #13483

Merged
merged 2 commits into from Jun 21, 2021
Merged

Don't hoist template literal keys in object-rest-spread #13483

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
89fffd5
remove hoisting when using template strings in proposal-object-rest-s…
lala7573 Jun 18, 2021
bf1fbc7
remove const from test
lala7573 Jun 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions packages/babel-plugin-proposal-object-rest-spread/src/index.js
View file
Open in desktop
Expand Up @@ -95,13 +95,15 @@ export default declare((api, opts) => {
const props = path.node.properties;
const keys = [];
let allLiteral = true;
let hasTemplateLiteral = false;

for (const prop of props) {
if (t.isIdentifier(prop.key) && !prop.computed) {
// since a key {a: 3} is equivalent to {"a": 3}, use the latter
keys.push(t.stringLiteral(prop.key.name));
} else if (t.isTemplateLiteral(prop.key)) {
keys.push(t.cloneNode(prop.key));
hasTemplateLiteral = true;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is kinda redundant, you could just set allLiteral = false (becase TemplateLiteral is most often not literal). Or remove this branch, replace the following condition's isLiteral with isImmutable, and then TemplateLiteral would fall through to the else.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because when allLiteral is false we need to inject the toPropertyKey helper. We don't need it if we have template literals.

} else if (t.isLiteral(prop.key)) {
keys.push(t.stringLiteral(String(prop.key.value)));
} else {
Expand All @@ -110,7 +112,7 @@ export default declare((api, opts) => {
}
}

return { keys, allLiteral };
return { keys, allLiteral, hasTemplateLiteral };
}

// replaces impure computed keys with new identifiers
Expand Down Expand Up @@ -156,7 +158,8 @@ export default declare((api, opts) => {
path.get("properties"),
path.scope,
);
const { keys, allLiteral } = extractNormalizedKeys(path);
const { keys, allLiteral, hasTemplateLiteral } =
extractNormalizedKeys(path);

if (keys.length === 0) {
return [
Expand All @@ -179,7 +182,7 @@ export default declare((api, opts) => {
} else {
keyExpression = t.arrayExpression(keys);

if (!t.isProgram(path.scope.block)) {
if (!hasTemplateLiteral && !t.isProgram(path.scope.block)) {
// Hoist definition of excluded keys, so that it's not created each time.
const program = path.findParent(path => path.isProgram());
const id = path.scope.generateUidIdentifier("excluded");
Expand Down
9 changes: 9 additions & 0 deletions ...t-spread/test/fixtures/object-rest/template-literal-allLiterals-true-no-hoisting/input.js
View file
Open in desktop
@@ -0,0 +1,9 @@
const example = () => {
const input = {};
const foo = 'foo';

const {
[`${foo}_bar`]: country,
...rest
} = input;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an object destructure. We need to test an object rest, which is (confusingly) when there's no const declaration:

Suggested change
const {
[`${foo}_bar`]: country,
...rest
} = input;
({
[`${foo}_bar`]: country,
...rest
} = input);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it? I tought both the desturcturing and object-rest-spread plugins transformed both the syntaxes, it just depends on which plugin you enabled.

This test is currently broken on main when targeting chrome 55 (which supports destructuring, but not object rest/spread):
https://babeljs.io/repl/build/main#?browsers=chrome%2055&build=&builtIns=false&corejs=3.6&spec=false&loose=false&code_lz=MYewdgzgLgBApgDwIYFsAOAbOMC8MAUAlLgHwwDeAUDDKJLAJZhoCuse5AvgNzW3jQYAMxAhcMAOQiQE3nzqCqNGgG0ABgBJy0zgH0ARkgBOagLoAufizBQjATwA0fGgDo3RuND6dxTVlF5OIA&presets=env

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am so confused by the split between the two plugins…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely consider merging them, relying on the targets top-level option to decide how much to compile on the plugin.

}
8 changes: 8 additions & 0 deletions ...-spread/test/fixtures/object-rest/template-literal-allLiterals-true-no-hoisting/output.js
View file
Open in desktop
@@ -0,0 +1,8 @@
const example = () => {
const input = {};
const foo = 'foo';
const {
[`${foo}_bar`]: country
} = input,
rest = babelHelpers.objectWithoutProperties(input, [`${foo}_bar`]);
};