Skip to content

convert @babel/helper-skip-transparent-expression-wrappers to typescript#13640

Closed
lightmare wants to merge 1 commit intobabel:mainfrom
lightmare:ts-helper-skip-transparent-expression-wrappers
Closed

convert @babel/helper-skip-transparent-expression-wrappers to typescript#13640
lightmare wants to merge 1 commit intobabel:mainfrom
lightmare:ts-helper-skip-transparent-expression-wrappers

Conversation

@lightmare
Copy link
Copy Markdown
Contributor

@lightmare lightmare commented Aug 5, 2021

Q                       A
Fixed Issues? n/a
Patch: Bug Fix? ✔️
Major: Breaking Change? not sure, but the types are stricter, so probably breaking
Minor: New Feature?
Tests Added + Pass? amended existing, none added
Documentation PR Link
Any Dependency Changes?
License MIT

@babel/helper-skip-transparent-expression-wrappers has a confusing interface. isTransparentExprWrapper takes a Node, while skipTransparentExprWrappers took a NodePath.
The latter was incorrectly called with a Node in @babel/plugin-proposal-optional-chainig:

expression = skipTransparentExprWrappers(expression);

Consequently, optional call in loose mode produced temp variables when it shouldn't, as in this test:
(_bar = ((_ref = (foo as A)).bar as B)) == null ? void 0 : _bar.call(_ref, foo.bar, false);

This PR overloads skipTransparentExprWrappers to accept either an Expression or NodePath<Expression>. This fixes the issue in @babel/plugin-proposal-optional-chaining, and also allows using the Expression overload in two other places in that plugin where the NodePath is not needed.

* overload skipTransparentExprWrappers

* fixes isSimpleMemberExpression
  in @babel/plugin-proposal-optional-chaining

* use skipTransparentExprWrappers<Expression> instead of the NodePath
  overload where possible in @babel/plugin-proposal-optional-chaining
@babel-bot
Copy link
Copy Markdown
Collaborator

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

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Aug 5, 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 61b6b16:

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

@fedeci fedeci added the Flow -> TS Tracking repository migration from Flow to TS label Aug 5, 2021
@nicolo-ribaudo
Copy link
Copy Markdown
Member

not sure, but the types are stricter, so probably breaking

They are just internal types, we don't publish them yet.

Comment on lines +30 to +34
export function skipTransparentExprWrappers(expr: t.Expression): t.Expression;

export function skipTransparentExprWrappers(
expr: NodePath<t.Expression>,
): NodePath<t.Expression>;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could also just expose two functions (skipTransparentExprWrappers and skipTransparentExprWrappersNodes); lets see what other team members think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree.

@lightmare Can you split this PR? One for JS->TS and the other with a new API? So we can merge the JS->TS in patch releases.

@lightmare lightmare deleted the ts-helper-skip-transparent-expression-wrappers branch November 9, 2021 18:18
@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 Feb 9, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2022
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.

5 participants