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 all 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
18 changes: 18 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 } from "@babel/types";

const renameVisitor: Visitor<Renamer> = {
ReferencedIdentifier({ node }, state) {
Expand All @@ -27,6 +28,23 @@ const renameVisitor: Visitor<Renamer> = {
}
},

ObjectProperty({ node, scope }, state) {
const { name } = node.key as Identifier;
if (
node.shorthand &&
// In destructuring the identifier is already renamed by the
// AssignmentExpression|Declaration|VariableDeclarator visitor,
// while in object literals it's renamed later by the
// ReferencedIdentifier visitor.
(name === state.oldName || name === state.newName) &&
// Ignore shadowed bindings
scope.getBindingIdentifier(name) === state.binding.identifier
) {
node.shorthand = false;
if (node.extra?.shorthand) node.extra.shorthand = false;
}
},

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

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

const renamedPropertyMatcher = expect.objectContaining({
type: "ObjectProperty",
shorthand: false,
extra: expect.objectContaining({ shorthand: false }),
key: expect.objectContaining({ name: "a" }),
value: expect.objectContaining({
name: expect.not.stringMatching(/^a$/),
}),
});

const { body } = program.node;
expect(body[0].declarations[0].id.properties[0]).toStrictEqual(
renamedPropertyMatcher,
);
expect(body[1].expression.left.properties[0]).toStrictEqual(
renamedPropertyMatcher,
);
expect(body[2].expression.right.properties[0]).toStrictEqual(
renamedPropertyMatcher,
);

expect(String(program)).toMatchInlineSnapshot(`
"const {
a: _a
} = b;
({
a: _a
} = b);
c = {
a: _a
};"
`);
});

it(".shorthand after renaming `ObjectProperty` - shadowed", () => {
const program = getPath(`
const a = 1;
{
const { b } = 2;
({ b } = 3);
(_ = { b });
}
`);
program.scope.rename("a", "b");

const originalPropertyMatcher = expect.objectContaining({
type: "ObjectProperty",
shorthand: true,
extra: expect.objectContaining({ shorthand: true }),
key: expect.objectContaining({ name: "b" }),
value: expect.objectContaining({ name: "b" }),
});
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


const { body } = program.node;
expect(body[1].body[0].declarations[0].id.properties[0]).toStrictEqual(
originalPropertyMatcher,
);
expect(body[1].body[1].expression.left.properties[0]).toStrictEqual(
originalPropertyMatcher,
);
expect(body[1].body[2].expression.right.properties[0]).toStrictEqual(
originalPropertyMatcher,
);

expect(String(program)).toMatchInlineSnapshot(`
"const b = 1;
{
const {
b
} = 2;
({
b
} = 3);
_ = {
b
};
}"
`);
});
});
});