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

Revert "Improve output when wrapping functions" #15979

Merged
merged 3 commits into from Sep 16, 2023

Conversation

jjonescz
Copy link
Contributor

@jjonescz jjonescz commented Sep 16, 2023

Q                       A
Fixed Issues? Fixes #15978
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

This adds a new test to reproduce the bug and reverts #15978 (without removing new tests added there). That's the simplest fix I could think of for now.
cc @liuxingbaoyu

@babel-bot
Copy link
Collaborator

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

@jjonescz jjonescz marked this pull request as ready for review September 16, 2023 13:06
@nicolo-ribaudo
Copy link
Member

Could we revert just the changes relative to function expressions and not to function declarations?

@jjonescz
Copy link
Contributor Author

Could we revert just the changes relative to function expressions and not to function declarations?

Do you mean that the optimization would apply to expressions but not declarations? Could that be done in a follow up PR - now only quickly fixing the regression by a safe revert of all changes?

@lightmare
Copy link
Contributor

I think the problem affects function declarations as well. If you replace the function expression in the example with a declaration, i.e. replace:

  for (const item of items) {
    await f(async (x) => {
      console.log(item);
    });
  }

with

  for (const item of items) {
    async function g(x) {
      console.log(item);
    }
    await f(g);
  }

the wrapper returned from _asyncToGenerator still cannot be cached in a function-scoped variable, because the wrapped function references a lexical.

@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 16, 2023

Ok let's revert this for now, we will reland with a proper fix later.

@nicolo-ribaudo nicolo-ribaudo changed the title Revert async wrapper optimization Revert "Improve output when wrapping functions" Sep 16, 2023
@nicolo-ribaudo nicolo-ribaudo added the PR: Revert ↩️ A type of pull request used for our changelog categories label Sep 16, 2023
@nicolo-ribaudo nicolo-ribaudo merged commit ad26f90 into babel:main Sep 16, 2023
48 checks passed
@jjonescz jjonescz deleted the 15978-AsyncCapture branch September 16, 2023 16:15
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Sep 16, 2023

Releasing, it will take 15 minutes

@liuxingbaoyu
Copy link
Member

Thank you for your PR! I have a complete refactor locally and I will reopen a PR later.

@overlookmotel
Copy link
Contributor

@liuxingbaoyu Just to flag that, depending on how you're refactoring, #15945 may also be similar - problem with temporary vars being function-scoped rather than block-scoped.

@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 Dec 18, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 18, 2023
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: Revert ↩️ A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: plugin-transform-async-to-generator incorrectly captures outer variable in a loop
6 participants