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

add skipTransparentExprWrapperNodes helper #13687

Merged

Conversation

@lightmare
Copy link
Contributor

@lightmare lightmare commented Aug 18, 2021

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:

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.

@babel-bot
Copy link
Collaborator

@babel-bot babel-bot commented Aug 18, 2021

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

@codesandbox
Copy link

@codesandbox codesandbox bot commented Aug 18, 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 8f26798:

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

@fedeci
Copy link
Member

@fedeci fedeci commented Aug 20, 2021

Hey @lightmare, can you rebase please?

* 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 Aug 20, 2021
fedeci
fedeci approved these changes Aug 20, 2021
@lightmare
Copy link
Contributor Author

@lightmare lightmare commented Aug 20, 2021

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
Contributor

@JLHwung 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
Contributor Author

@lightmare 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 added 2 commits Aug 20, 2021
* 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
Contributor Author

@lightmare 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
Member

@fedeci 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
Contributor Author

@lightmare lightmare commented Sep 20, 2021

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

@JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 20, 2021

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

@JLHwung JLHwung merged commit 99a3fef into babel:main Oct 28, 2021
26 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants