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

feat: support export star commonjs #85

Merged
merged 4 commits into from
Oct 20, 2023
Merged

feat: support export star commonjs #85

merged 4 commits into from
Oct 20, 2023

Conversation

underfin
Copy link
Contributor

@underfin underfin commented Oct 20, 2023

Description

Because commonjs export symbol can't static analyzer, it is caused to can't link symbol from the export star from commonjs. So esbuild use __reExport to make sure the commonjs export symbol can binding to importer module namespace object.

Here is a esbuild example https://esbuild.github.io/try/#YgAwLjE5LjMALS1idW5kbGUgLS1zcGxpdHRpbmcgLS1vdXRkaXI9ZGlzdCAtLWZvcm1hdD1lc20AZQBlbnRyeS5tanMAaW1wb3J0ICogYXMgcyBmcm9tICcuL2EnAABhLmpzAGV4cG9ydCAqIGZyb20gJy4vYycKZXhwb3J0IGNvbnN0IGEgPSAxAABjLmpzAGV4cG9ydHMuZm9vID0gZnVuY3Rpb24oKSB7CiAgICByZXR1cm4gJ2JhcicKfQAAZW50cnkxLmpzAGltcG9ydCBhIGZyb20gJy4vYS5qcycKAABkLmpzAAovL2V4cG9ydCAqIGFzIGEgZnJvbSAnLi9hJwovL2V4cG9ydCAqIGZyb20gJy4vYycKZXhwb3J0IGNvbnN0IGQgPSAxCgo.

The pr related to #36 and also enable some esbuild test.

Test Plan

Adde.


@@ -29,8 +26,10 @@ impl<'ast> EsmSourceRender<'ast> {
})
.collect::<Vec<_>>()
.join(",\n");
self.ctx.source.append(format!("\nvar {namespace_name} = {{\n{exports}\n}};\n",));
self.ctx.source.prepend(format!("\nvar {namespace_name} = {{\n{exports}\n}};\n",));
Copy link
Contributor Author

@underfin underfin Oct 20, 2023

Choose a reason for hiding this comment

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

I moved the namespace variable declaration to first for esm render, it is becuase __reExport will access the namespace symbol, we need to make sure it is initialized. Also the syntax semantic is not changed.

crates/rolldown/src/bundler/graph/linker.rs Outdated Show resolved Hide resolved
@@ -47,7 +46,67 @@ impl<'graph> Linker<'graph> {
});
}

fn wrap_modules_if_needed(&mut self) {
fn create_symbols_for_import_wrapped_module(&mut self) {

This comment was marked as outdated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think initialize_runtime_symbols_for_importers_of_wrapped_modules is not right, it also create other symbols

Copy link
Member

Choose a reason for hiding this comment

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

initialize_facade_symbols_for_importers_of_wrapped_modules. Also notice that for_importers_of_wrapped_modules is a more accurate than for_import_wrapped_module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think, because import is verb inside import_wrapped_module, it is better than for_importers_of_wrapped_modules, here has extra preposition.

Copy link
Member

Choose a reason for hiding this comment

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

if import is a verb, you need to add ing while putting it before the "for".

"import_wrapped_module" is a behavior not a thing, which is confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about create_symbols_for_wrapped_module_imported?

Copy link
Member

Choose a reason for hiding this comment

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

Not good. It's eve wrong. You are creating symbols for modules that import the wrapped module not the imported wrapped module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @Boshen

Copy link
Member

@hyf0 hyf0 Oct 20, 2023

Choose a reason for hiding this comment

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

Allow me to add some descriptions to explain why initialize_facade_symbols_for_importers_of_wrapped_modules is a more accurate name.

What's a wrapped module?

For a commonjs module, which has source code

// foo.cjs
module.exports = { a: 1 }

Rolldown will converted it to the following code

// foo.cjs
const require_foo = __commonJS({
  "foo.cjs"(exports, module)  {
     module.exports = { a: 1 }
  }
})

So, foo.cjs is a wrapped module, wrapped by "__commonJS({ ... })".

For module that import the foo.cjs, which will be transformed from

// foo.js
import * as foo from './foo.cjs'
console.log(foo)

to

// main.js
var foo_ns = __toESM(require_foo());
console.log(foo_ns)

You will notice that there is a __toESM symbol, which is created by rolldown and is done by this line of code.


What the function create_symbols_for_import_wrapped_module do is to iterate all modules. For each module, the function will find it's imported module. If the imported module is a wrapped module, we will create such as the "__toESM" in the each module, which is the importer of the wrapped module.

So the name initialize_facade_symbols_for_importers_of_wrapped_modules describe what the function do accurately.

This also tells that how the name create_symbols_for_wrapped_module_imported is not only just about the accuracy but even wrong.


I don't consider initialize_facade_symbols_for_importers_of_wrapped_modules a perfect name, but it's at least a better name.

@hyf0
Copy link
Member

hyf0 commented Oct 20, 2023

From my experience, there are two ways to do the naming. One is you give a detailed naming to describe what the function do. Another one is to use a shorthand name with comments that describe what the function do.

@underfin underfin merged commit 07b8c03 into main Oct 20, 2023
4 checks passed
@underfin underfin deleted the commonjs-reexport branch October 20, 2023 12:19
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.

2 participants