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

Set shorthand: false when renaming an identifier inside an object property #15649

Merged
merged 5 commits into from May 29, 2023
Merged

Set shorthand: false when renaming an identifier inside an object property #15649

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e4d4746
feature: babel-traverse: set shorthand when rename Identifier inside …
coderaiser May 25, 2023
d6c01d3
feature: babel-traverse: shorthand of ObjectProperty (#15648)
coderaiser May 26, 2023
961233c
test: babel-preset-env: update fixture
coderaiser May 26, 2023
e039d8e
Fix bug
nicolo-ribaudo May 29, 2023
a7728f2
misc
liuxingbaoyu May 29, 2023
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
4 changes: 2 additions & 2 deletions packages/babel-preset-env/test/fixtures/preset-options/destructuring-edge/output.js
View file
Open in desktop
@@ -1,8 +1,8 @@
((a, {
b: _b = 0,
c: _c = 3
c = 3
Copy link
Member

Choose a reason for hiding this comment

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

This code should be transformed, because Edge 15 doesn't support default values in destructuring in parameters of arrow functions.

I don't understand why the first property is still transformed, but the second one isn't 🤔

The source code of this transform is https://github.com/babel/preset-modules/blob/master/src/plugins/transform-edge-default-parameters/index.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like the reason is setting:

path.parent.shorthand = false;
(path.parent.extra || {}).shorthand = false;

And if shorthand set to false no fix for ObjectProperty occurred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also first version of my PR didn't affect additional fixtures, should I revert to previous version?

@liuxingbaoyu have you test this case locally? My tests with "additional traversing" worked good.

}) => {
return a === 1 && _b === 2 && _c === 3;
return a === 1 && _b === 2 && c === 3;
})(1, {
b: 2
});
17 changes: 17 additions & 0 deletions packages/babel-traverse/src/scope/lib/renamer.ts
View file
Open in desktop
Expand Up @@ -5,6 +5,7 @@ import type { NodePath, Visitor } from "../..";
import { requeueComputedKeyAndDecorators } from "@babel/helper-environment-visitor";
import { traverseNode } from "../../traverse-node";
import { explode } from "../../visitors";
import type { Identifier, ObjectProperty } from "@babel/types";

const renameVisitor: Visitor<Renamer> = {
ReferencedIdentifier({ node }, state) {
Expand All @@ -26,6 +27,22 @@ const renameVisitor: Visitor<Renamer> = {
}
}
},
ObjectProperty(path: NodePath<ObjectProperty>) {
Copy link
Member

Choose a reason for hiding this comment

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

In this function we should also check if value.name === state.oldName, because we only want to modify the nodes that we are renaming.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it doesn't matter? Because this should not happen normally in AST.

Also technically, even value.name === state.oldName is not necessarily the node we just renamed. So I tend not to check it.

const { extra, key, value, computed, shorthand } = path.node;

if (computed) {
return;
}

if (!shorthand) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

When shorthand===true, must computed===false? I'm not entirely sure about this, any other reviewers know? Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Uh yes, shorthand properties cannot be computed.


if ((key as Identifier).name !== (value as Identifier).name) {
path.node.shorthand = false;
if (extra?.shorthand) extra.shorthand = false;
}
},

"AssignmentExpression|Declaration|VariableDeclarator"(
path: NodePath<t.AssignmentPattern | t.Declaration | t.VariableDeclarator>,
Expand Down
53 changes: 53 additions & 0 deletions packages/babel-traverse/test/scope.js
View file
Open in desktop
Expand Up @@ -1038,5 +1038,58 @@ describe("scope", () => {
},
});
});

it(".shorthand after renaming `ObjectProperty ` in `ObjectPattern`", () => {
const program = getPath(`
const { a } = b;
({ a } = b);
`);

program.traverse({
Identifier(path) {
if (path.node.name !== "a") return;

path.scope.rename("a");
},
});
Copy link
Member

Choose a reason for hiding this comment

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

Could:

  • Move the assertion from inside the visitor to after the program.traverse call, to be 100% sure that it runs (I know you copied this pattern from the test above, but we can do better 🙂)
  • Replace .toBeFalsy() with .toBe(false) (since we expect false and not any falsy value)
  • Add an expect(program.toString()).toMatchInlineSnapshot() at the end, which helps when reading the tests to more easily see what's happening

Also, it would be good to have the following tests:

  • const a = 1, obj = { a };, renaming a
  • const { a } = 1; { const { b } = 2 } renaming a to b

Copy link
Member

Choose a reason for hiding this comment

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

I did another fix locally.
I don't think it's necessary to traverse twice to fix this, just add the ObjectProperty visitor.
At the same time I also found a strange thing, we also have an extra.shorthand in the AST, I don't understand why.🤔
image


expect(
t.cloneDeepWithoutLoc(
program.node.body[0].declarations[0].id.properties[0],
),
).toMatchInlineSnapshot(`
Object {
"computed": false,
"extra": Object {
"shorthand": false,
},
"key": Object {
"loc": null,
"name": "a",
"type": "Identifier",
},
"loc": null,
"shorthand": false,
"type": "ObjectProperty",
"value": Object {
"extra": Object {},
"loc": null,
"name": "_a",
"type": "Identifier",
},
}
`);
expect(
t.cloneDeepWithoutLoc(program.node.body[0].declarations[1]),
).toMatchInlineSnapshot(`undefined`);
expect(program + "").toMatchInlineSnapshot(`
"const {
a: _a
} = b;
({
a: _a
} = b);"
`);
});
});
});