Skip to content

Conversation

@Gianthard-cyh
Copy link

@Gianthard-cyh Gianthard-cyh commented Jul 14, 2025

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.

@coderabbitai
Copy link

coderabbitai bot commented Jul 14, 2025

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link

github-actions bot commented Jul 15, 2025

Size Report

Bundles

File Size Gzip Brotli
compiler-dom.global.prod.js 84.9 kB 29.8 kB 26.3 kB
runtime-dom.global.prod.js 107 kB 40.2 kB 36.2 kB
vue.global.prod.js 165 kB 60.1 kB 53.6 kB

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47.9 kB 18.7 kB 17.1 kB
createApp 57 kB 21.9 kB 20 kB
createApp + vaporInteropPlugin 90.6 kB 33.8 kB 30.6 kB
createVaporApp 34.3 kB 13.1 kB 12 kB
createSSRApp 61.3 kB 23.7 kB 21.6 kB
defineCustomElement 62.6 kB 23.6 kB 21.5 kB
overall 72.1 kB 27.3 kB 24.9 kB

@pkg-pr-new
Copy link

pkg-pr-new bot commented Jul 15, 2025

Open in StackBlitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@13630

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@13630

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@13630

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@13630

@vue/compiler-vapor

npm i https://pkg.pr.new/@vue/compiler-vapor@13630

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@13630

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@13630

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@13630

@vue/runtime-vapor

npm i https://pkg.pr.new/@vue/runtime-vapor@13630

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@13630

@vue/shared

npm i https://pkg.pr.new/@vue/shared@13630

vue

npm i https://pkg.pr.new/vue@13630

@vue/compat

npm i https://pkg.pr.new/@vue/compat@13630

commit: 3501d47

@Gianthard-cyh
Copy link
Author

Hi! @edison1105
I've committed a version that replaces JSON.stringify with a custom escape function, and wraps the template strings with backticks (`) instead of double quotes ("). I bundled and tested the output and it successfully fixes the issue.

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.

@edison1105 edison1105 linked an issue Jul 16, 2025 that may be closed by this pull request
@edison1105 edison1105 moved this to Vapor in Next Minor Jul 16, 2025
@ShenQingchuan
Copy link
Contributor

The current implementation of this PR still has the following issues:

  1. The template string parameter within the generated _template(...) template creation has an incorrect implementation. It converts to _template(``<img src="" + _imports_0 + "" alt="..." >``), which breaks the src attribute's value.

  2. Currently, transformNativeElement has a fundamental problem:

interface IREffect {
  // Here, `expressions` was previously considered to only be `SimpleExpressionNode` in its type.
  expressions: (SimpleExpressionNode | CompoundExpressionNode)[]
  operations: OperationNode[]
}

Therefore, for srcset's multiple import string variable concatenating operations, it cannot be properly parsed and recognized, leading to it ultimately being classified as a dynamic expression and thus unable to be statically hoisted.

Thus, transformNativeElement needs to categorize and discuss the expression types to determine whether they can be statically hoisted as a whole:

  • simple expression / string / symbol = can be directly hoisted
  • compound expression = recursive judgment
  • interpolation expression = judge whether the interpolated expression can be hoisted

@Gianthard-cyh
Copy link
Author

Thanks for your response @ShenQingchuan! I’ve also encountered this with assetURLs. Since IRProp is currently typed to only accept SimpleExpressionNode:

export interface IRProp extends Omit<DirectiveTransformResult, 'value'> {
  values: SimpleExpressionNode[]
}

…it becomes tricky to support cases like:

<use href="~@svg/file.svg#fragment"></use>

In this case, the prop value is actually _imports_0 + '#fragment', but it’s wrapped as a single SimpleExpressionNode, even though it’s clearly a compound expression. This makes it difficult to correctly extract or manipulate the individual parts, especially for asset URL resolution.

I’m currently trying to work around this, though it’s a bit challenging for me.

@Gianthard-cyh Gianthard-cyh requested a review from edison1105 July 18, 2025 15:53
@netlify
Copy link

netlify bot commented Oct 20, 2025

Deploy Preview for vue-sfc-playground failed. Why did it fail? →

Name Link
🔨 Latest commit 079bc73
🔍 Latest deploy log https://app.netlify.com/projects/vue-sfc-playground/deploys/68f6f53782d18600088043c8

@edison1105 edison1105 changed the title fix(compiler-vapor): fix asset import from public directory feat(compiler-vapor): handle asset imports Nov 7, 2025
@edison1105
Copy link
Member

Thank you for your PR. The general approach is correct. However, some of the changes can be simplified. I've done some refactoring, see https://github.com/vuejs/core/compare/f63ad44..0467b95

@Gianthard-cyh
Copy link
Author

Thank you for your PR. The general approach is correct. However, some of the changes can be simplified. I've done some refactoring, see https://github.com/vuejs/core/compare/f63ad44..0467b95

Thanks for the refactor and for your patience! I’m still new here and learning a lot. Let me know if there’s anything else I should work on 🙇‍♂️

@edison1105 edison1105 merged commit 7d4ab91 into vuejs:minor Nov 10, 2025
13 of 14 checks passed
@github-project-automation github-project-automation bot moved this from Vapor to Done in Next Minor Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: compiler scope: vapor related to vapor mode

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Vapor] Using image from public directory causes error

3 participants