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

Convert @babel/helper-explode-assignable-expression to ts #12417

Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Nov 27, 2020

Q                       A
License MIT

See review comments below

@JLHwung JLHwung added PR: Internal 🏠 A type of pull request used for our changelog categories Flow -> TS Tracking repository migration from Flow to TS labels Nov 27, 2020
if (t.isPrivateName(prop)) {
throw new Error(
"We can't generate property ref for private name, please install `@babel/plugin-proposal-class-properties`",
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already throws when private name is in property (REPL)

Property right of AssignmentExpression expected node to be of a type ["Expression"] but instead got "PrivateName"

Now we throws polished error message.

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 27, 2020

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

@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 27, 2020

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 bf43768:

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

let ref;
if (t.isSuper(node)) {
// Super cannot be directly assigned so lets return it directly
return node;
Copy link
Contributor Author

@JLHwung JLHwung Nov 27, 2020

Choose a reason for hiding this comment

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

This is unreachable per Babel's usage:

const exploded = explode(node.left, nodes, this, scope);

The left of an assignment expression can never be a Super.

Even if anyone passes a Super here, it will throw in

nodes.push(t.assignmentExpression("=", t.cloneNode(temp), t.cloneNode(prop)));

because t.cloneNode(node.property) is undefined and the AssignmentExpression constructor will reject undefined as input. So we better throw early that Super can not be exploded.

} {
let obj;
if (t.isIdentifier(node) && allowedSingleIdent) {
obj = node;
} else {
obj = getObjRef(node, nodes, file, scope);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

file is unused. I leave it in public interface for compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a TODO(Babel 8), so we don't forget about removing it.

@@ -28,7 +36,7 @@ function getObjRef(node, nodes, file, scope) {
return ref;
}
} else {
throw new Error(`We can't explode this node type ${node.type}`);
throw new Error(`We can't explode this node type ${node["type"]}`);
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise TS will think this branch is unreachable and type property is not defined in never.

} {
let obj;
if (t.isIdentifier(node) && allowedSingleIdent) {
obj = node;
} else {
obj = getObjRef(node, nodes, file, scope);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a TODO(Babel 8), so we don't forget about removing it.

@JLHwung
Copy link
Contributor Author

JLHwung commented Dec 1, 2020

Converted to draft since type checking it depends on transition of @babel/traverse to be done.

@JLHwung JLHwung marked this pull request as draft December 1, 2020 15:24
@nicolo-ribaudo nicolo-ribaudo removed the PR: Internal 🏠 A type of pull request used for our changelog categories label Dec 10, 2020
@nicolo-ribaudo
Copy link
Member

@JLHwung @babel/traverse has been merged!

@JLHwung JLHwung force-pushed the move-helper-explode-assignable-expression branch from b117ce8 to e901434 Compare January 25, 2021 22:46
@JLHwung JLHwung marked this pull request as ready for review January 25, 2021 22:46
@nicolo-ribaudo nicolo-ribaudo changed the title refactor: move @babel/helper-explode-assignable-expression to ts Convert @babel/helper-explode-assignable-expression to ts Feb 20, 2021
@nicolo-ribaudo nicolo-ribaudo merged commit 62c1ccb into babel:main Feb 20, 2021
@nicolo-ribaudo nicolo-ribaudo deleted the move-helper-explode-assignable-expression branch February 20, 2021 23:11
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 23, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Flow -> TS Tracking repository migration from Flow to TS outdated A closed issue/PR that is archived due to age. Recommended to make a new issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants