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

Fix shorthand identifier import usage #9222

Merged
merged 8 commits into from
Sep 15, 2023

Conversation

jondlm
Copy link
Contributor

@jondlm jondlm commented Aug 29, 2023

↪️ Pull Request

Fixes #9140 by adding a new SWC visitor to the JS transformer. It visits all identifiers, checks if their IDs match any of the previously detected imports. If so it adds them to the used_imports set.

💻 Examples

See #9140 for an outline of the bug this fixes.

🚨 Test instructions

I added an integration test that fails before this change and passes after. I couldn't find unit tests for the JS transformer so I'm assuming an integration test is appropriate here. I used @lettertwo's new fsFixture helper to colocate the test files with the actual test. That worked quite nicely!

✔️ PR Todo

  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

//
// import { foo } from "bar";
// const baz = { foo };
if self.imports.contains_key(&id!(node)) {
Copy link
Contributor Author

@jondlm jondlm Aug 29, 2023

Choose a reason for hiding this comment

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

I couldn't think of any cases where this new visitor would be problematic since SWC seems to imply that checking the id here helps us uniquely identify the correct use accounting for scope. Let me know if I'm wrong! I am very new to using Rust with SWC.

@jondlm
Copy link
Contributor Author

jondlm commented Sep 7, 2023

Perhaps @mischnic could take a look at this one when you have time?

Copy link
Member

@mischnic mischnic left a comment

Choose a reason for hiding this comment

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

Seems like object shorthand is one of the few cases where there's an actual identifier that isn't/can't be an expression (all of the other cases are handled by visit_expr)

@devongovett devongovett merged commit ac96136 into parcel-bundler:v2 Sep 15, 2023
15 of 16 checks passed
@jondlm jondlm deleted the jdlm-issue-9140 branch September 15, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing export with sideEffects: false
4 participants