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

feat: treat required module with ExportKind::None as commonjs #319

Merged
merged 1 commit into from
Nov 19, 2023
Merged

feat: treat required module with ExportKind::None as commonjs #319

merged 1 commit into from
Nov 19, 2023

Conversation

hyf0
Copy link
Member

@hyf0 hyf0 commented Nov 18, 2023

Description

Test Plan

This was referenced Nov 18, 2023
Merged
Merged
@hyf0
Copy link
Member Author

hyf0 commented Nov 18, 2023 •
edited

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@hyf0 hyf0 mentioned this pull request Nov 18, 2023
Merged
hyf0
hyf0 commented Nov 18, 2023
View reviewed changes
crates/rolldown/tests/esbuild/default/require_main_cache_common_js/artifacts.snap
@@ -8,7 +8,7 @@ input_file: crates/rolldown/tests/esbuild/default/require_main_cache_common_js
## entry_js.mjs

```js
import { __commonJS } from "./_rolldown_runtime.mjs";
import { __commonJS, __toESM } from "./_rolldown_runtime.mjs";
Copy link
Member Author

Choose a reason for hiding this comment

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

Unused __toESM generated because referencing symbols by module type not actually usages.

Related code:

https://github.com/rolldown-rs/rolldown/blob/94f5ddd7d8b5f02006bb17cfdd95544f989fe376/crates/rolldown/src/bundler/linker/linker.rs#L238-L258

hyf0
hyf0 commented Nov 18, 2023
View reviewed changes
crates/rolldown/tests/fixtures/mix-cjs-esm/artifacts.snap
const value = 1;
// esm-import-cjs-export.js
var require_esm_import_cjs_export = __commonJS({
'esm-import-cjs-export.js'(exports, module) {
init_foo();
var import_foo$1 = __toESM(require_foo());
Copy link
Member Author

Choose a reason for hiding this comment

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

Should beimport_foo instead of import_foo$1. This cause by the same reason, we generated symbols that we don't used.

This was referenced Nov 18, 2023
Merged
Merged
Merged
Merged
Merged
@underfin
Copy link
Contributor

I looked into the code, but have a question about your pr fix.

const ABC = 1;

With your implementation, the module will have ExportKind::None, but the module generate code is not wrapped with __commonJs.

Can you point to me what i missed?

underfin
underfin reviewed Nov 19, 2023
View reviewed changes
crates/rolldown/src/bundler/stages/link_stage.rs
@@ -97,6 +114,217 @@ impl LinkStage {
"runtime module should always be the first module in the sorted modules"
);
}

fn determine_module_exports_kind(&mut self) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why here need to prepare determine module exports kind? I'm not understand to this, can you explain to it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course. The reason is we need to determine the ExportsKind of modules, we need the information while wrapping the modules.

@hyf0
Copy link
Member Author

hyf0 commented Nov 19, 2023

I looked into the code, but have a question about your pr fix.

const ABC = 1;

With your implementation, the module will have ExportKind::None, but the module generate code is not wrapped with __commonJs.

Can you point to me what i missed?

I looked into the code, but have a question about your pr fix.

const ABC = 1;

With your implementation, the module will have ExportKind::None, but the module generate code is not wrapped with __commonJs.

Can you point to me what i missed?

https://github.com/rolldown-rs/rolldown/pull/319/files#diff-eef021adf45cf3a638e3dbb95392baf7706bb3367c6465c74375b36f5709232eR133-R144

required module with ExportKind:None would be considered as commonjs module.

@hyf0 hyf0 changed the title feat: treat module with ExportKind::None as commonjs feat: treat required module with ExportKind::None as commonjs Nov 19, 2023
This was referenced Nov 19, 2023
Merged
Merged
Merged
Merged
underfin
underfin reviewed Nov 19, 2023
View reviewed changes
crates/rolldown/src/bundler/stages/link_stage.rs

module.import_records.iter().for_each(|rec| {
wrap_module(ctx, rec.resolved_module);
if rec.kind.is_static() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

here look like has a error

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Wonder why clippy doesn't catch this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's me fix it in another PR. Too much PRs to be restacked now.

@hyf0
Copy link
Member Author

hyf0 commented Nov 19, 2023 •
edited

Merge activity

  • Nov 19, 6:40 AM: @hyf0 started a stack merge that includes this pull request via Graphite.
  • Nov 19, 6:40 AM: Graphite rebased this pull request as part of a merge.
  • Nov 19, 6:41 AM: @hyf0 merged this pull request with Graphite.

Base automatically changed from 11-17-feat_empty_module_should_have_ExportsKind_None_ to main November 19, 2023 11:40
@hyf0 hyf0 force-pushed the 11-18-feat_treat_module_with_ExportKind_None_as_commonjs branch from c76267b to 126378c Compare November 19, 2023 11:40
@hyf0 hyf0 merged commit 0738ad8 into main Nov 19, 2023
6 checks passed
@hyf0 hyf0 deleted the 11-18-feat_treat_module_with_ExportKind_None_as_commonjs branch November 19, 2023 11:41
@hyf0 hyf0 mentioned this pull request Nov 19, 2023
Merged
hyf0 added a commit that referenced this pull request Nov 19, 2023
@hyf0
fix: only import needed runtime symbols (#324)
51db31f 
<!-- Thank you for contributing! -->

### Description

The fix is done by more delicate analyzing the module type and import kind.

This also fixes #319 (comment).

<!-- Please insert your description here and provide especially info about the "what" this PR is solving -->

### Test Plan

<!-- e.g. is there anything you'd like reviewers to focus on? -->

---
@hyf0 hyf0 mentioned this pull request Nov 19, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@underfin underfin underfin approved these changes

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@hyf0 @underfin