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.

Sign up for GitHub

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

Jump to bottom

fix: reflect-metadata import order #18086

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix: reflect-metadata import order #18086

wants to merge 8 commits into from

Conversation

DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Mar 11, 2025 •
edited
Loading

What does this PR do?

Closes #4677

Fixes module resolution and importing with these changes:

  • record when imported modules declare themselvesas "type": "commonjs"
  • "type": "commonjs" is now used as a hint by the transpiler to treat code being parsed as CJS (and to add a CJS wrapper function around it)
  • when asynchronously importing a cjs module (e.g. from an ESM module), those CJS modules now get transpiled and loaded synchronously

What was happening?

Tl;Dr, reflect-metadata was being treated as ESM and imports to it were resolving after tsyringe imports.

How did you verify your code works?

I wrote a test

@DonIsaac DonIsaac requested a review from Jarred-Sumner March 11, 2025 23:50
@robobun
Copy link

robobun commented Mar 11, 2025 •
edited
Loading

Updated 8:35 PM PT - Mar 11th, 2025

❌ @DonIsaac, your commit 2a44c4e has 88 failures in Build #13076:

🧪   try this PR locally:

bunx bun-pr 18086

@DonIsaac DonIsaac changed the title add test fix: synchronously load CJS modules imported from ESM Mar 11, 2025
@DonIsaac DonIsaac changed the title fix: synchronously load CJS modules imported from ESM fix: reflect-metadata import order Mar 12, 2025
@DonIsaac
Copy link
Contributor Author

update: we've decided to go with the hacky approach that doesn't force sync transpilation of cjs modules. Only reflect-metadata will be transpiled synchronously

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jarred-Sumner Jarred-Sumner Awaiting requested review from Jarred-Sumner

At least 0 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reflect-metadata doesn't work for tsyringe
2 participants
@DonIsaac @robobun