Skip to content

[WIP] Fix dynamic imports not transformed to require calls in CJS #7276

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
Draft
Copilot wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

[WIP] Fix dynamic imports not transformed to require calls in CJS #7276

Copilot wants to merge 4 commits into from
+198 −5

Conversation

Copy link
Contributor

Copilot AI commented Dec 1, 2025 •
edited
Loading

  • Analyze the issue and understand the requirements
  • Review the maintainer's comment and previous related issue [Feature Request]: Support output.dynamicImportInCjs: false #2548
  • Explore the codebase to understand how dynamic imports are currently handled
  • Build the project to verify it compiles successfully
  • Implement dynamicImportInCjs option in BundlerOptions (Rust)
  • Implement dynamicImportInCjs option in NormalizedBundlerOptions (Rust)
  • Implement dynamicImportInCjs option in binding_output_options (Rust NAPI bindings)
  • Implement dynamicImportInCjs option in OutputOptions (TypeScript)
  • Update module finalizer to check dynamicImportInCjs option before transforming dynamic imports
  • Add require_call_expr helper method to AstSnippet
  • Add tests for dynamicImportInCjs: true (keeps import())
  • Add tests for dynamicImportInCjs: false (transforms to require())
  • Add tests for external modules with dynamicImportInCjs: false
  • Update CLI snapshot for the new option
  • Run linting and tests to verify the implementation
  • Run code review and security checks
Original prompt

This section details on the original issue you should resolve

<issue_title>Dynamic imports not transformed to require calls when format is cjs</issue_title>
<issue_description>### Reproduction link or steps

Reproduction: https://github.com/TimoGlastra/tsdown-dynamic-import. Instructions on how to run in readme.

What is expected?

Source code is written as:

export async function run() {
  const myLib = await import("some-lib");
}

run();

With tsdown format set the cjs. I would expect the dynamic import to be rewritten as a dynamic require:

//#region src/index.ts
async function run() {
  require("some-lib");
}
run();

//#endregion
exports.run = run;

What is actually happening?

The dynamic import is kept when format is CJS:

//#region src/index.ts
async function run() {
  await import("some-lib");
}
run();

//#endregion
exports.run = run;

Any additional comments?

I was able to work around it with a hacky plugin. But this doesn't feel like a good solution.

We are importing this in React Native, and with the TSC compiler we used require. Now that we are migrating to ESM we are updating all dynamic requires to dynamic imports. But we would like the CJS output to match the previous behvaiour to make the migration to ESM as smooth as possible.

</issue_description>

Comments on the Issue (you are @copilot in this section)

@sxzz Previous issue: #2548 /cc @shulaoda

Additionally, even if you enable dynamicImportInCjs, Rolldown will not convert your asynchronous code to synchronous, as this is unsafe. Furthermore, using import or require with package.json#exports can resolve to different files, which also poses risks.

You should manually use createRequire or require as needed.</comment_new>

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@graphite-app
Copy link
Contributor

graphite-app bot commented Dec 1, 2025

How to use the Graphite Merge Queue

Add the label graphite: merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@netlify
Copy link

netlify bot commented Dec 1, 2025 •
edited
Loading

❌ Deploy Preview for rolldown-rs failed.

Name Link
🔨 Latest commit 59c54ad
🔍 Latest deploy log https://app.netlify.com/projects/rolldown-rs/deploys/692dc237d5434a000838c980

Copilot AI requested a review from shulaoda December 1, 2025 03:26
@shulaoda
Copy link
Member

shulaoda commented Dec 1, 2025

@copilot try again

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2025

Benchmarks Rust

  • target: main(6dc1191)
  • pr: copilot/fix-dynamic-import-transformation(e3c57ed)
group                                                        pr                                     target
-----                                                        --                                     ------
bundle/bundle@multi-duplicated-top-level-symbol              1.02     67.2±1.60ms        ? ?/sec    1.00     65.9±1.78ms        ? ?/sec
bundle/bundle@multi-duplicated-top-level-symbol-sourcemap    1.02     74.1±1.71ms        ? ?/sec    1.00     72.8±2.26ms        ? ?/sec
bundle/bundle@rome_ts                                        1.01    111.1±3.74ms        ? ?/sec    1.00    110.3±4.29ms        ? ?/sec
bundle/bundle@rome_ts-sourcemap                              1.02    124.5±2.15ms        ? ?/sec    1.00    122.0±2.00ms        ? ?/sec
bundle/bundle@threejs                                        1.00     40.1±0.78ms        ? ?/sec    1.01     40.4±0.80ms        ? ?/sec
bundle/bundle@threejs-sourcemap                              1.01     44.4±0.70ms        ? ?/sec    1.00     43.9±0.90ms        ? ?/sec
bundle/bundle@threejs10x                                     1.00    403.4±5.31ms        ? ?/sec    1.00    401.9±4.39ms        ? ?/sec
bundle/bundle@threejs10x-sourcemap                           1.00    463.4±4.59ms        ? ?/sec    1.01    467.7±5.16ms        ? ?/sec
scan/scan@rome_ts                                            1.00     86.4±1.80ms        ? ?/sec    1.01     87.4±1.84ms        ? ?/sec
scan/scan@threejs                                            1.00     29.2±0.44ms        ? ?/sec    1.01     29.4±1.84ms        ? ?/sec
scan/scan@threejs10x                                         1.00    304.1±5.43ms        ? ?/sec    1.00    304.1±4.79ms        ? ?/sec

Copy link
Contributor Author

Copilot AI commented Dec 1, 2025

@copilot try again

Fixed in 59c54ad - updated the CLI snapshot test to include the new --dynamic-import-in-cjs option. All tests pass now.

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

Reviewers

@shulaoda shulaoda Awaiting requested review from shulaoda

At least 1 approving review is required to merge this pull request.

Assignees

@shulaoda shulaoda

Copilot code review Copilot

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Dynamic imports not transformed to require calls when format is cjs

2 participants

@shulaoda