Skip to content

add skipTransparentExprWrapperNodes helper#13687

Merged
JLHwung merged 3 commits intobabel:mainfrom
lightmare:ts-helper-skip-transparent-expression-wrappers-p2
Oct 28, 2021
Merged

add skipTransparentExprWrapperNodes helper#13687
JLHwung merged 3 commits intobabel:mainfrom
lightmare:ts-helper-skip-transparent-expression-wrappers-p2

Conversation

@lightmare
Copy link
Copy Markdown
Contributor

@lightmare lightmare commented Aug 18, 2021 •
edited by gitpod-io bot
Loading

Requires #13686

Q                       A
Fixed Issues? Closes #13640, this is the second half
Patch: Bug Fix? ✔️
Major: Breaking Change? ➖
Minor: New Feature? ➖
Tests Added + Pass? amended existing
Documentation PR Link
Any Dependency Changes?
License MIT

Description copied from #13640 :

@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-chaining:

babel/packages/babel-plugin-proposal-optional-chaining/src/transform.js

Line 11 in b2d9156

expression = skipTransparentExprWrappers(expression);

Consequently, optional call in loose mode produced temp variables when it shouldn't, as in this test:

babel/packages/babel-plugin-proposal-optional-chaining/test/fixtures/transparent-expr-wrappers/ts-as-function-call-loose/output.js

Line 3 in b2d9156

(_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.

@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented Aug 18, 2021 •
edited
Loading

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

@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Aug 18, 2021 •
edited
Loading

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 8f26798:

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

@fedeci
Copy link
Copy Markdown
Member

fedeci commented Aug 20, 2021

Hey @lightmare, can you rebase please?

@lightmare
overload skipTransparentExprWrappers
fad4759 
* fixes isSimpleMemberExpression
  in @babel/plugin-proposal-optional-chaining

* use skipTransparentExprWrappers<Expression> instead of the NodePath
  overload where possible in @babel/plugin-proposal-optional-chaining
@lightmare lightmare force-pushed the ts-helper-skip-transparent-expression-wrappers-p2 branch from f422c3c to fad4759 Compare August 20, 2021 10:53
@fedeci fedeci added this to the v7.16.0 milestone Aug 20, 2021
@fedeci fedeci added PR: New Feature 🚀 A type of pull request used for our changelog categories area: helpers labels Aug 20, 2021
@lightmare
Copy link
Copy Markdown
Contributor Author

lightmare commented Aug 20, 2021 •
edited
Loading

There's an E2E failure that seems related, it throws: Cannot read property 'isOptionalMemberExpression' of undefined

It looks like skipTransparentExprWrappers returned undefined at some point.

The only explanation I could come up with is that the expr instanceof NodePath check fails (I have no idea why, when given a path from traverse), then it goes into the else branch, where isTransparentExprWrapper(expr) succeeds because it effectively checks expr.type and NodePath happens to have a .type member, and then expr.expression is returned, which NodePath doesn't have.

But it's weird, this is not the only place where instanceof NodePath is used.

@JLHwung
Copy link
Copy Markdown
Contributor

JLHwung commented Aug 20, 2021

The expr instanceof NodePath check may fail when there are multiple copies of @babel/traverse in node modules and some of them are depended by different packages. We have seen similar issues in the past: #11734

@lightmare
Copy link
Copy Markdown
Contributor Author

lightmare commented Aug 20, 2021

Thank you for the explanation. I guess the best solution will be to add a separate method instead of overloading.

@lightmare
Revert "overload skipTransparentExprWrappers"
3ec1256 
This reverts commit fad4759.
@lightmare
add helper skipTransparentExprWrapperNodes
8f26798 
* avoids constructing NodePaths when only the Node is needed

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

* use this new helper instead of skipTransparentExprWrappers
  where possible in @babel/plugin-proposal-optional-chaining
@lightmare
Copy link
Copy Markdown
Contributor Author

lightmare commented Aug 20, 2021

I have reverted the overload patch, and pushed a new version with a separate function.
When it's okay, I will squash the commits so that the reverted patch doesn't clutter history.

@fedeci
Copy link
Copy Markdown
Member

fedeci commented Aug 21, 2021

Didn't we "recently" merged another helper with instanceof NodePath in its body?

@JLHwung JLHwung changed the title overload skipTransparentExprWrappers for NodePath | Expression add skipTransparentExprWrapperNodes helper Sep 20, 2021
@lightmare
Copy link
Copy Markdown
Contributor Author

lightmare commented Sep 20, 2021

Should I squash the commits in the branch, or will you just squash it during merge?

@JLHwung
Copy link
Copy Markdown
Contributor

JLHwung commented Sep 20, 2021

@lightmare We use Squash and merge most of time. So you are all set.

@nicolo-ribaudo nicolo-ribaudo added the PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release label Sep 21, 2021
@JLHwung JLHwung merged commit 99a3fef into babel:main Oct 28, 2021
@snyk-bot snyk-bot mentioned this pull request Dec 14, 2021
Closed
@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 Jan 28, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 28, 2022
@lightmare lightmare deleted the ts-helper-skip-transparent-expression-wrappers-p2 branch September 2, 2023 18:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@JLHwung JLHwung JLHwung approved these changes

@fedeci fedeci fedeci approved these changes

Assignees

No one assigned

Labels

area: helpers outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories PR: Ready to be Merged A pull request with already two approvals, but waiting for the next minor release

Projects

None yet

Milestone

v7.16.0

Development

Successfully merging this pull request may close these issues.

5 participants

@lightmare @babel-bot @fedeci @JLHwung @nicolo-ribaudo