Skip to content

Don't use args rest/spread to hoist super method calls#9939

Merged
nicolo-ribaudo merged 3 commits intobabel:masterfrom
nicolo-ribaudo:issue-9935
Oct 11, 2019
Merged

Don't use args rest/spread to hoist super method calls#9939
nicolo-ribaudo merged 3 commits intobabel:masterfrom
nicolo-ribaudo:issue-9935

Conversation

@nicolo-ribaudo
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo commented May 4, 2019 •
edited
Loading

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

I don't fully know super.foo() semantics, but I think that this PR is correct?

@nicolo-ribaudo nicolo-ribaudo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label May 4, 2019
@babel-bot
Copy link
Copy Markdown
Collaborator

babel-bot commented May 4, 2019 •
edited
Loading

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

nicolo-ribaudo
nicolo-ribaudo commented May 4, 2019
View reviewed changes
...sform-async-to-generator/test/fixtures/async-to-generator/object-method-with-super/output.js Outdated

return babelHelpers.asyncToGenerator(function* () {
_superprop_callMethod();
_superprop_getMethod().call(this);

This comment was marked as resolved.

Sign in to view

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request May 24, 2019
Closed
@edoardocavazza
Copy link
Copy Markdown

edoardocavazza commented Oct 9, 2019

Hi @nicolo-ribaudo! Is something blocking this PR? Can I help some way?

@nicolo-ribaudo
Copy link
Copy Markdown
Member Author

nicolo-ribaudo commented Oct 9, 2019

It needs reviews 😛
If you could go through my code, check if it looks correct (and also ask questions for everything that isn't clear!), and than approve this PR it would be really appreciated.
We have a two-✔️ policy, but I can try to find another reviewers.

@edoardocavazza
Copy link
Copy Markdown

edoardocavazza commented Oct 10, 2019

@nicolo-ribaudo I can't see the approve button (maybe I need to be selected as reviewer?) but I saw your latest changes and, IMHO, everything is ok! Thank you!

jridgewell
jridgewell approved these changes Oct 11, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Should we rewrite this to use helper-replace-supers?

@nicolo-ribaudo
Copy link
Copy Markdown
Member Author

nicolo-ribaudo commented Oct 11, 2019

@jridgewell helper-replace-supers always transpile super, while this helper makes it possible to hoist it without transpiling it.

@nicolo-ribaudo nicolo-ribaudo merged commit 99035ca into babel:master Oct 11, 2019
@nicolo-ribaudo nicolo-ribaudo deleted the issue-9935 branch October 11, 2019 08:18
@jridgewell
Copy link
Copy Markdown
Member

jridgewell commented Oct 11, 2019

Right, we would replace the super expressions with calls to the hoisted helpers. 😉

@nicolo-ribaudo nicolo-ribaudo mentioned this pull request Oct 22, 2019
Closed
@snyk-bot snyk-bot mentioned this pull request Nov 14, 2019
Merged
This was referenced Nov 28, 2019
Open
Open
This was referenced Dec 5, 2019
Open
Merged
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jan 10, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jridgewell jridgewell jridgewell approved these changes

+1 more reviewer

@edoardocavazza edoardocavazza edoardocavazza approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Using super in async method

4 participants

@nicolo-ribaudo @babel-bot @edoardocavazza @jridgewell