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.

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

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

Merged
merged 2 commits into from Jun 21, 2021

Conversation

@lala7573
Copy link
Contributor

@lala7573 lala7573 commented Jun 18, 2021

Q                       A
Fixed Issues? Fixes #13430
Patch: Bug Fix? 👍
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? 👍
Documentation PR Link
Any Dependency Changes?
License MIT

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Jun 18, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46977/

@codesandbox
Copy link

@codesandbox codesandbox bot commented Jun 18, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bf1fbc7:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

const {
[`${foo}_bar`]: country,
...rest
} = input;
Comment on lines 5 to 8

This comment has been minimized.

@jridgewell

jridgewell Jun 18, 2021
Member

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);

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 18, 2021
Member

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

This comment has been minimized.

@jridgewell

jridgewell Jun 21, 2021
Member

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

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 21, 2021
Member

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

@lala7573 lala7573 requested a review from jridgewell Jun 21, 2021

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;

This comment has been minimized.

@lightmare

lightmare Jun 21, 2021
Contributor

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.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Jun 21, 2021
Member

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

@nicolo-ribaudo nicolo-ribaudo changed the title remove hoisting when using template strings in proposal-object-rest-s… Don't hoist template literal keys in object-rest-spread Jun 21, 2021
@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Jun 21, 2021

I edited the title so that it fits in a commit message header

@nicolo-ribaudo nicolo-ribaudo merged commit 8ae0efe into babel:main Jun 21, 2021
25 of 27 checks passed
25 of 27 checks passed
@github-actions
Prepare Cache
Details
@github-actions
Validate Yarn dependencies and constraints
Details
@github-actions
Test on Node.js Latest
Details
@github-actions
Build Babel Artifacts
Details
@github-actions
Test Babel 8 breaking changes
Details
@github-actions
Publish to local Verdaccio registry
Details
@github-actions
Lint
Details
@github-actions
Test on Node.js (14)
Details
@github-actions
Test on Node.js (12)
Details
@github-actions
Test on Node.js (10)
Details
@github-actions
Test on Node.js (8)
Details
@github-actions
Test on Node.js (6)
Details
@github-actions
Test on Windows
Details
@github-actions
Third-party Parser Tests
Details
@github-actions
Test @babel/runtime integrations
Details
@github-actions
E2E (babel)
Details
@github-actions
E2E (babel-old-version)
Details
@github-actions
E2E (create-react-app)
Details
@github-actions
E2E (vue-cli)
Details
@github-actions
E2E (jest)
Details
@circleci-checks
e2e-breaking-pr Workflow: e2e-breaking-pr
Details
@circleci-checks
test262-pr Workflow: test262-pr
Details
@gitpod-io
Gitpod Open an online workspace in Gitpod
Details
@babel-bot
babel/repl REPL preview is available
Details
@circleci-checks
build-standalone Workflow: build-standalone
Details
@codesandbox
ci/codesandbox Building packages succeeded.
Details
@codecov
codecov/project 92.05% (target 90.00%)
Details
nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Jul 30, 2021
* remove hoisting when using template strings in proposal-object-rest-spread

* remove const from test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants