fix(core): allow assetPrefix to be an empty string#6971
fix(core): allow assetPrefix to be an empty string#6971chenjiahan merged 11 commits intoweb-infra-dev:mainfrom
Conversation
✅ Deploy Preview for rsbuild-v2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Summary of ChangesHello @xun082, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in development mode where a global Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where dev.assetPrefix would override an environment-specific output.assetPrefix in development mode, which is crucial for multi-environment setups, especially for Node.js targets that require relative asset paths. The changes in getPublicPath correctly prioritize the environment-specific configuration and also ensure that an empty assetPrefix is preserved. My review includes a suggestion to refactor a related utility function for better long-term maintainability.
…alid relative path
…082/rsbuild into fix/environment-assetprefix-override
… comments on empty string preservation
chenjiahan
left a comment
There was a problem hiding this comment.
I'm concerned this change will break SSR hydration: #6539 (comment)
Thanks for the concern. I've fixed it: when server.base is set, server targets now use it to match the client's public path for SSR hydration, while still preserving empty string for worker_threads when server.base is not set. |
…y dev mode behavior
chenjiahan
left a comment
There was a problem hiding this comment.
This PR seems to break down into two parts:
- Allowing
assetPrefixto be an empty string. This change makes sense to me, and I’d be happy to see this part merged first. - Automatically adjusting
assetPrefixfor the Node target. This part feels a bit too complex, and I find it difficult to clearly explain the behavior in the documentation, so I'm a bit cautious about moving forward with this change.
Thanks for the feedback! I’ll simplify the changes by keeping the first part (allowing an empty assetPrefix) and removing the automatic adjustment for Node targets. However, there are E2E test failures related to Node target manifest paths — something I haven’t worked with in E2E tests before, so I’m unfamiliar with this error. Do you have any insight into the cause? Should we update the tests to match the new behavior, or do you suggest a different approach for handling manifest paths for Node targets? |
|
I will check the failed E2E cases and try to fix them |
|
Thank you! I've merged the first part changes |
Summary
Fixes an issue where
dev.assetPrefixconfiguration leaks to Node environment in multi-environment setup, causing Worker imports to fail with "Cannot find module" errors.Problem:
dev.assetPrefixis set (e.g.,/1.0.0/), environment-specificoutput.assetPrefixis ignoredError: Cannot find module '/1.0.0/src_server_worker_ts.js'Solution:
output.assetPrefixnow takes priority over globaldev.assetPrefixin dev modeassetPrefixis preserved without formatting to enable relative paths for node targetsChanges:
getPublicPath()function inpackages/core/src/plugins/output.tsoutput.assetPrefix(if non-default) →dev.assetPrefix→ defaultRelated Links
Closes #6539
Checklist