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 filename function #5957

Merged
merged 17 commits into from
Mar 21, 2024
Merged

Conversation

branchseer
Copy link
Contributor

@branchseer branchseer commented Mar 15, 2024

Summary

Allow filename to be a function in these places:

  • output.filename
  • output.chunkFilename
  • output.cssFilename
  • output.cssChunkFilename
  • compilation.getPath(filename, data)
  • compilation.getPathWithInfo(filename, data)
  • compilation.getAssetPath(filename, data)
  • compilation.getAssetPathWithInfo(filename, data)

Fixes #3483.

Most changed files in the PR are refactoring (in)direct callers of Filename::render due to its return type changed from String to rspack_error::Result<String> (functions could throw).

Other notable changes:

  • Add type ByRef that implements Hash and Eq by js reference.
    This makes it possible for filename functions to be hashmap keys.
  • Add type ThreadSafeFunctionWithRef that can be called in the node thread without causing deadlocks.
    This is to avoid Filename::render becoming async. The reason I choose to make Filename::render blocking instead of async is async would infect all the (in)direct callers, and require much more refactors than changing the return type. I don't think that's worth the effort since js filename functions are usually very lightweight.
  • Allow entry to be a function.
    In webpack tests, most tests for filename functions also defines entry as function, not object. We have to support this to unlock them.

Require Documentation?

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the release: feature release: feature related release(mr only) label Mar 15, 2024
@h-a-n-a h-a-n-a self-assigned this Mar 15, 2024
@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Mar 15, 2024

Thanks for the contribution! I will get you back when I'm free ;-)

# Conflicts:
#	crates/rspack_core/src/compiler/compilation.rs
#	crates/rspack_plugin_devtool/src/lib.rs
# Conflicts:
#	crates/rspack_plugin_devtool/src/lib.rs
# Conflicts:
#	crates/rspack_core/src/compiler/compilation.rs
@h-a-n-a
Copy link
Collaborator

h-a-n-a commented Mar 20, 2024

@ahabhgk Would you please help double check the API part at your convenience?

h-a-n-a
h-a-n-a previously approved these changes Mar 20, 2024
Copy link
Collaborator

@h-a-n-a h-a-n-a left a comment

Choose a reason for hiding this comment

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

Thank you so much for the wonderful work you've done! We really appreciate your contribution ;-)

Copy link
Collaborator

@ahabhgk ahabhgk left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, one small nitpick

@hardfist
Copy link
Contributor

@branchseer you can add document change in this pr, since we have moved the website repo into rspack repo

Copy link

netlify bot commented Mar 21, 2024

👷 Deploy request for rspack pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2d0e035

Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for rspack-back canceled.

Name Link
🔨 Latest commit 2d0e035
🔍 Latest deploy log https://app.netlify.com/sites/rspack-back/deploys/65fc255d13150c00081ad2c9

@branchseer
Copy link
Contributor Author

@branchseer you can add document change in this pr, since we have moved the website repo into rspack repo

@hardfist done in b38608a.

# Conflicts:
#	crates/rspack_plugin_library/src/assign_library_plugin.rs
Copy link
Collaborator

@ahabhgk ahabhgk left a comment

Choose a reason for hiding this comment

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

Thanks!

@ahabhgk ahabhgk enabled auto-merge (squash) March 21, 2024 12:57
@ahabhgk ahabhgk merged commit 67df93d into web-infra-dev:main Mar 21, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release: feature release: feature related release(mr only)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow output.filename to be a function
6 participants