Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.8k
fix(compiler-vapor): fix asset import from public directory#13630
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:minor
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
Size ReportBundles
Usages
|
@vue/compiler-core@vue/compiler-dom@vue/compiler-sfc@vue/compiler-ssr@vue/compiler-vapor@vue/reactivity@vue/runtime-core@vue/runtime-dom@vue/runtime-vapor@vue/server-renderer@vue/sharedvue@vue/compatcommit: |
| @@ -1,5 +1,17 @@ | |||
| // Vitest Snapshot v1, https://vitest.dev/guide/snapshot.html | |||
| exports[`compile > asset imports > from public directory 1`] = ` | |||
| "import { setProp as _setProp, renderEffect as _renderEffect, template as _template } from 'vue'; | |||
| import _imports_0 from '/vite.svg'; | |||
edison1105Jul 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
import_imports_0from'/vite.svg';constt0=_template(`<img src="${_imports_0}">`,true)
It should be done like this to avoidrenderEffect andsetProp.
Gianthard-cyhJul 15, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Hi! Thanks for the suggestion — I’ve tried to move_imports_0 into the template string as recommended to avoidrenderEffect andsetProp.
core/packages/compiler-vapor/src/generators/template.ts
Lines 7 to 20 in65abd6e
| exportfunctiongenTemplates( | |
| templates:string[], | |
| rootIndex:number|undefined, | |
| { helper}:CodegenContext, | |
| ):string{ | |
| returntemplates | |
| .map( | |
| (template,i)=> | |
| `const t${i} =${helper('template')}(${JSON.stringify( | |
| template, | |
| )}${i===rootIndex ?', true' :''})\n`, | |
| ) | |
| .join('') | |
| } |
However, thegenTemplates function still usesJSON.stringify(), which wraps the entire template in double quotes and escapes inner quotes. That prevents me from generating template literals like:
constt0=_template(`<img src="${_imports_0}">`,true)
I also tried string concatenation:
template+=`${key.content}=""+${values[0].content}+""`
But this results in a normal string with escaped quotes.
constt0=_template("<img src=\"\"+_imports_0+\"\">")
Let me know if you have any thoughts or preferred direction on this — happy to collaborate on the final approach!
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.
It should be easier to implement using string concatenation. For example, like this:
- Wrap the variable with special characters
template+=`${key.content}="$\{${values[0].content}\}$"`
- In genTemplates, replace the variable with string concatenation
${JSON.stringify(template).replace(/\${_imports_(\d+)}\$/g,'"+ _imports_$1 +"')
Hi!@edison1105 This change breaks the existing unit tests because previous snapshots were based on template strings wrapped with double quotes. I'm not sure whether this change might break anything else beyond the existing tests, so I’d appreciate your review and feedback before proceeding further. |
| @@ -262,4 +263,46 @@ describe('compile', () => { | |||
| ) | |||
| }) | |||
| }) | |||
| describe('asset imports', () => { | |||
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.
A new test file should be created to testassetUrl andsrcset. For example:
- templateTransformAssetUrl.spec.ts
- templateTransformSrcset.spec.ts
Try to port the relevant test cases from the existingtemplateTransformAssetUrl.spec.ts andtemplateTransformSrcset.spec.ts.
ShenQingchuan commentedJul 17, 2025
The current implementation of this PR still has the following issues:
interfaceIREffect{// Here, `expressions` was previously considered to only be `SimpleExpressionNode` in its type.expressions:(SimpleExpressionNode|CompoundExpressionNode)[]operations:OperationNode[]} Therefore, for Thus,
|
Thanks for your response@ShenQingchuan! I’ve also encountered this with assetURLs. Since exportinterfaceIRPropextendsOmit<DirectiveTransformResult,'value'>{values:SimpleExpressionNode[]} …it becomes tricky to support cases like: <usehref="~@svg/file.svg#fragment"></use> In this case, the prop value is actually I’m currently trying to work around this, though it’s a bit challenging for me. |
Uh oh!
There was an error while loading.Please reload this page.
Problem statement
See#13623.
Change summary
Add
importsfor TransformContext in compiler-vapor.Modify expression generator to treat the expression of imported public asset properly.
Generate asset imports.
Closes#13623.