-
-
Notifications
You must be signed in to change notification settings - Fork 8.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 our terms of service and privacy 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 with CodeRabbit:
SupportNeed help? Create a ticket on our support 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/shared
vue
@vue/compat
commit: |
@@ -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'; |
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_0 from '/vite.svg';
const t0 = _template(`<img src="${_imports_0}">`, true)
It should be done like this to avoid renderEffect
and setProp
.
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 avoid renderEffect
and setProp
.
core/packages/compiler-vapor/src/generators/template.ts
Lines 7 to 20 in 65abd6e
export function genTemplates( | |
templates: string[], | |
rootIndex: number | undefined, | |
{ helper }: CodegenContext, | |
): string { | |
return templates | |
.map( | |
(template, i) => | |
`const t${i} = ${helper('template')}(${JSON.stringify( | |
template, | |
)}${i === rootIndex ? ', true' : ''})\n`, | |
) | |
.join('') | |
} |
However, the genTemplates
function still uses JSON.stringify()
, which wraps the entire template in double quotes and escapes inner quotes. That prevents me from generating template literals like:
const t0 = _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.
const t0 = _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!
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. |
Problem statement
See #13623.
Change summary
Add
imports
for TransformContext in compiler-vapor.Modify expression generator to treat the expression of imported public asset properly.
Generate asset imports.
Closes #13623.