-
Notifications
You must be signed in to change notification settings - Fork 503
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
feat: treat required module with ExportKind::None
as commonjs
#319
feat: treat required module with ExportKind::None
as commonjs
#319
Conversation
@@ -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"; |
There was a problem hiding this comment.
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:
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()); |
There was a problem hiding this comment.
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.
I looked into the code, but have a question about your pr fix. const ABC = 1; With your implementation, the module will have Can you point to me what i missed? |
@@ -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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
|
ExportKind::None
as commonjsExportKind::None
as commonjs
|
||
module.import_records.iter().for_each(|rec| { | ||
wrap_module(ctx, rec.resolved_module); | ||
if rec.kind.is_static() {} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
c76267b
to
126378c
Compare
<!-- 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? --> ---
Description
Test Plan