Skip to content

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Apr 24, 2025

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

This PR fixed two issues:

  1. The amd/cjs/umd transform will throw when the local exports reference nested var declarations:

    export { A }
    {
      var A
    }

    This pattern is not very popular in real-world codebase but the explicit-resource-management transform will generate such pattern.

  2. Preserve class id for inner class binding references, as suggested by @overlookmotel in plugin-proposal-explicit-resource-management incorrect transform for exported class #17172. Fixing this issue relies on the first fix.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories area: modules Spec: Explicit Resource Management labels Apr 24, 2025
@babel-bot
Copy link
Collaborator

babel-bot commented Apr 24, 2025

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

// If localName is not found in the bindingKindLookup generated
// from top level declarations, it must be a reference to a var
// declaration defined within block statement or switch case
const kind = bindingKindLookup.get(localName) ?? "var";
Copy link
Member

Choose a reason for hiding this comment

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

Even with export { A }; { var A }, the programPath.scope.get("A") properly returns the var declaration. Could we just query the scope properly, so that we still catch invalid exports?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The invalid exports have been caught by the parser. Assuming the previous check somehow detects nested var, now if it fails, there should be a plugin injecting new exports without defining them.

If we remove bindingKindLookup and query from the program scope, chances are plugins that injects export { A }; var A; will fail because the plugin does not update the program scope. So we should keep bindingKindLookup and fallback to scope query.

@nicolo-ribaudo
Copy link
Member

Node.js 6 is still unhappy :)

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks for the scope-based update!

@JLHwung JLHwung requested a review from liuxingbaoyu May 15, 2025 19:47
@JLHwung JLHwung merged commit 1d4546b into babel:main May 18, 2025
57 checks passed
@JLHwung JLHwung deleted the fix-17172 branch May 18, 2025 02:42
@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 Aug 17, 2025
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: modules 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: Explicit Resource Management
Projects
None yet
Development

Successfully merging this pull request may close these issues.

plugin-proposal-explicit-resource-management incorrect transform for exported class
4 participants