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

Memoize call expressions in optional chains in loose mode #11261

Merged
merged 4 commits into from Mar 20, 2020

Conversation

oliverdunk
Copy link
Contributor

Q                       A
Fixed Issues? Fixes #11084
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

Previously, in loose mode, we weren't memoizing optional call expressions. This resulted in code like a.get(b)?.(); being compiled to a.get(b) == null ? void 0 : a.get(b)();, which repeats a function call.

This MR changes the behaviour so that memoization is only skipped when no work is done by the call's callee.

@oliverdunk oliverdunk changed the title Memoize in loose mode when callee is a CallExpression babel-plugin-proposal-optional-chaining/src/index.js: Memoize in loose mode when callee is a CallExpression Mar 14, 2020
@oliverdunk oliverdunk changed the title babel-plugin-proposal-optional-chaining/src/index.js: Memoize in loose mode when callee is a CallExpression babel-plugin-proposal-optional-chaining: Memoize in loose mode when callee is a CallExpression Mar 14, 2020
Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

I think we need more test here. Eg,

foo.bar()?.()
foo[bar]()?.()

foo.bar().baz?.()
foo[bar]().baz?.()

This will highlight a few more cases we're failing. To fix, we need to adjust the isCall checks below.

@oliverdunk
Copy link
Contributor Author

@jridgewell Please ignore that request for review. I need to make sure we're probably setting the call context.

@nicolo-ribaudo nicolo-ribaudo added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Optional Chaining labels Mar 15, 2020
Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

If you need help, here's another test case.

@oliverdunk
Copy link
Contributor Author

@jridgewell Found and fixed my bug! It would be great if you could take a look :)

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Almost there.

@oliverdunk
Copy link
Contributor Author

@jridgewell I've addressed your feedback again. Really appreciate all of the help! It's early days when it comes to my understanding of Babel - but you and Nico have been incredibly welcoming.

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

Nice work @oliverdunk!

Copy link
Member

@kaicataldo kaicataldo left a comment

Choose a reason for hiding this comment

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

Thank you for contributing!

@nicolo-ribaudo
Copy link
Member

Thanks Oliver!

@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 Jun 20, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2020
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: Optional Chaining
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[plugin-proposal-optional-chaining] Loose mode produces unoptimized code for class methods
5 participants