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

Handle cases when ?? and ?. is in binding initializers #12032

Merged
merged 8 commits into from Oct 3, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Sep 3, 2020

Q                       A
Fixed Issues? Fixes #11166, fixes #12026, closes #11533
Patch: Bug Fix? Yes
Tests Added + Pass? Yes
License MIT

This PR is inspired from #11533 but tries to solve it without interface changes to scope#maybeGenerateMemoized. It also addressed #11533 (comment).

Todo:

  • Add tests for loose mode

@babel-bot
Copy link
Collaborator

babel-bot commented Sep 3, 2020

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

@JLHwung JLHwung marked this pull request as ready for review September 4, 2020 19:38
@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 4, 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 dc257bf:

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

// Replace `function (a, x = a.b?.#c) {}` to `function (a, x = (() => a.b?.#c)() ){}`
// so the temporary variable can be injected in correct scope
// This can be further optimized to avoid unecessary IIFE
if (scope.path.isPattern()) {
Copy link
Member

Choose a reason for hiding this comment

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

Does this only apply in parameters? I'm not sure when a scope'a path would be the pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this only apply in parameters?

Yes, if a Pattern is a sopce.path, its parent must be a Function:

// If a Pattern is an immediate descendent of a Function, it must be in the params.

because a Pattern is not Scopeable.

However when reviewing what is authored by past me in #10912, I do realize that current isScope definition does not cover pattern in catch clause:

try { } catch ({ e = foo?.bar }) {}

I will fix that in a separate PR.

optionalPath.get("object") ?? optionalPath.get("callee"),
);
if (node.optional) {
return !scope.isStatic(childPath.node);
Copy link
Member

Choose a reason for hiding this comment

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

I think we should only be returning if it's non-static. If it is static, we should continue the loop. Eg, foo?.bar?.baz().qux needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I have added foo?.bar?.baz().qux as an example. I overlooked the fact that NodePath#get always returns non-nullish.

If childPath.node is static, it must be this, super or identifier and we could stop here since optionalPath.isOptionalChain() will be no longer true in the next loop. The idea here is to find the last chain before ? an optional chain. So in your example childPath.node will be foo?.bar for foo?.bar?.baz().qux. foo?.bar is non-static so the whole chain will then be wrapped in IIFE.

We could further optimize and allow SimpleMemberExpression for loose mode.

@JLHwung JLHwung merged commit 5f83a8c into babel:main Oct 3, 2020
@JLHwung JLHwung deleted the optional-chaining-in-params branch October 3, 2020 14:04
@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 3, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Class Fields Spec: Nullish Coalescing Operator Spec: Optional Chaining
Projects
None yet
5 participants