Skip to content

feat: add dynamic_import_vars plugin#1732

Merged
underfin merged 32 commits intorolldown:mainfrom
kermanx:feat/plugin-dynamic-import-vars
Aug 1, 2024
Merged

feat: add dynamic_import_vars plugin#1732
underfin merged 32 commits intorolldown:mainfrom
kermanx:feat/plugin-dynamic-import-vars

Conversation

@kermanx
Copy link
Copy Markdown
Contributor

@kermanx kermanx commented Jul 25, 2024

Listed in #819.

This PR implements Vite's dynamic-import-vars plugin as a built-in Rolldown plugin.

Todos

  1. The tests still fail because import.meta.glob in the source code can be transformed, but those generated by dynamic_import_vars_plugin cannot. I don't know how to address this.

  2. This PR has to copy nodes, but I can't find the right way to do this in Rust (related to CloneIn trait for AST nodes oxc-project/oxc#4284?). Currently, the AST is first generated to JS and then parsed, in order to be cloned.

  3. Other TODOs are rather less important and have comments starting with TODO: in the source code.

Notes

In Vite, the raw import expression is first resolved as a path and then converted to a glob pattern. However, because the raw import expression is in AST form in Rolldown, it is first converted to a glob pattern and then resolved as a path.

@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 25, 2024

Deploy Preview for rolldown-rs canceled.

Name Link
🔨 Latest commit 87efa4f
🔍 Latest deploy log https://app.netlify.com/sites/rolldown-rs/deploys/66ab393ab9188a00083a79b2

@hyf0
Copy link
Copy Markdown
Member

hyf0 commented Jul 25, 2024

Thanks for the PR. We will review this when we plan to implement this feature.

@underfin underfin self-assigned this Jul 25, 2024
@kermanx kermanx marked this pull request as draft July 25, 2024 16:47
*expr = self.ast_builder.expression_call(
import_expr.span,
self.ast_builder.vec1(
self.ast_builder.argument_expression(clone_expr(self.ast_builder, &import_expr.source)),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here the &import_expr.source is not be used again, you could take it out form import_expr to avoid clone.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The import_expr is a reference, thus it can't be moved into Argument 🤔

Copy link
Copy Markdown
Contributor

@underfin underfin Aug 1, 2024

Choose a reason for hiding this comment

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

Here it is a &mut import_expr, i will using to move it out.

std::mem::replace(
            &mut import_expr.source,
            self.ast_builder.expression_null_literal(SPAN),
          )

Comment thread crates/rolldown_plugin_dynamic_import_vars/src/to_glob.rs Outdated
Comment thread crates/rolldown_plugin_dynamic_import_vars/src/lib.rs Outdated
@underfin
Copy link
Copy Markdown
Contributor

Wrong resolved path in dynamic imports

Yeah. It is our internal implementation for import path lookup problem, we using span to store some info with ast, we are try using astId instead of it. We could leave it at now.

For other to do list, we could add some TODO comment at code, it will leave something todo for others. Thank you.

@kermanx
Copy link
Copy Markdown
Contributor Author

kermanx commented Jul 29, 2024

The tests still fail - import.meta.glob in the source code can be transformed, but those generated by dynamic_import_vars_plugin cannot. I don't know how to address this.

Other TODOs mentioned above are now in code comments.

@kermanx kermanx marked this pull request as ready for review July 29, 2024 09:23
Comment thread crates/rolldown_plugin_dynamic_import_vars/src/lib.rs Outdated
@underfin
Copy link
Copy Markdown
Contributor

The tests still fail - import.meta.glob in the source code can be transformed, but those generated by dynamic_import_vars_plugin cannot. I don't know how to address this.

I will see it, thank you.

@IWANABETHATGUY
Copy link
Copy Markdown
Member

@underfin This pull request has been opened for about one week, maybe we could merge it first if it has no big issue? And polish it in the future?

@underfin
Copy link
Copy Markdown
Contributor

I could see it at next, please wait a while.

@underfin underfin enabled auto-merge (squash) August 1, 2024 07:22
@underfin
Copy link
Copy Markdown
Contributor

underfin commented Aug 1, 2024

Thank your awesome contribute.

@underfin
Copy link
Copy Markdown
Contributor

underfin commented Aug 1, 2024

The tests still fail - import.meta.glob in the source code can be transformed, but those generated by dynamic_import_vars_plugin cannot. I don't know how to address this.

Other TODOs mentioned above are now in code comments.

Here because the global import plugin not handle nested call expression, i using walk_mut::walk_expression(self, expr); to resolve it.

auto-merge was automatically disabled August 1, 2024 07:28

Head branch was pushed to by a user without write access

@underfin underfin merged commit 40cf58b into rolldown:main Aug 1, 2024
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.

5 participants