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

fix(plugin-vue): allow overwriting template.transformAssetUrls.includeAbsolute (fix #4836) #6779

Merged
merged 7 commits into from
May 6, 2022
Merged

fix(plugin-vue): allow overwriting template.transformAssetUrls.includeAbsolute (fix #4836) #6779

merged 7 commits into from
May 6, 2022

Conversation

innocenzi
Copy link
Contributor

Description

This PR fixes an edge-case of @vitejs/plugin-vue where absolute URLs would go through the asset plugin, causing build-time crashes.

Additional context

In most cases, that would be the expected behavior, but when integrating with back-end frameworks, it's most likely not because they use a different public directory, not handled by Vite.

Generally, we don't want assets in there to be versioned, and there is potentially not even the referenced asset at build-time. In either case, this causes build-time errors.

For instance, say we have a public/logo.svg and reference it with <img src="/logo.svg" />. Using a back-end framework, we don't want Vite to process this file. In development, this works fine. But at build-time, we get a Rollup error:

> vite build

vite v2.7.13 building for production...
✓ 19 modules transformed.
[vite]: Rollup failed to resolve import "/logo.svg" from "resources\views\pages\welcome.vue".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
error during build:
Error: [vite]: Rollup failed to resolve import "/logo.svg" from "resources\views\pages\welcome.vue".
This is most likely unintended because it can break your application at runtime.
If you do want to externalize this module explicitly add it to
`build.rollupOptions.external`
    at onRollupWarning (D:\Code\http\laravel\node_modules\.pnpm\[email protected]\node_modules\vite\dist\node\chunks\dep-f5552faa.js:37915:19)
    at onwarn (D:\Code\http\laravel\node_modules\.pnpm\[email protected]\node_modules\vite\dist\node\chunks\dep-f5552faa.js:37693:13)
    at Object.onwarn (D:\Code\http\laravel\node_modules\.pnpm\[email protected]\node_modules\rollup\dist\shared\rollup.js:23116:13)
    at ModuleLoader.handleResolveId (D:\Code\http\laravel\node_modules\.pnpm\[email protected]\node_modules\rollup\dist\shared\rollup.js:22396:26)
    at D:\Code\http\laravel\node_modules\.pnpm\[email protected]\node_modules\rollup\dist\shared\rollup.js:22357:26
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
[!] Error: unfinished hook action(s) on exit:
(vite:esbuild) transform "D:/Code/http/laravel/resources/scripts/vite/import-page-component.ts"
(vite:load-fallback) load "D:/Code/http/laravel/node_modules/.pnpm/@[email protected]/node_modules/@vue/runtime-core/dist/runtime-core.esm-bundler.js"
(vite:load-fallback) load "D:/Code/http/laravel/node_modules/.pnpm/@[email protected]/node_modules/@vue/shared/dist/shared.esm-bundler.js"
(commonjs) load "\u0000D:/Code/http/laravel/node_modules/.pnpm/[email protected]/node_modules/axios/index.js?commonjs-proxy"
(commonjs) load "\u0000D:/Code/http/laravel/node_modules/.pnpm/[email protected]/node_modules/qs/lib/index.js?commonjs-proxy"
(commonjs) load "\u0000D:/Code/http/laravel/node_modules/.pnpm/[email protected]/node_modules/deepmerge/dist/cjs.js?commonjs-proxy"
(vite:load-fallback) load "D:/Code/http/laravel/node_modules/.pnpm/[email protected]/node_modules/axios/index.js"
(vite:load-fallback) load "D:/Code/http/laravel/node_modules/.pnpm/[email protected]/node_modules/qs/lib/index.js"
(vite:load-fallback) load "D:/Code/http/laravel/node_modules/.pnpm/[email protected]/node_modules/deepmerge/dist/cjs.js"

This PR fixes that issue by providing a new option, resolve.absoluteUrls. The reason behind setting this parameter is because we don't want users to have to specify a big option object themselves:

vue({
  template: {
	transformAssetUrls: {
	  includeAbsolute: true
    }
  }
})

Rather, we want this auto-configured in a plugin. The setting may be useful to other plugins too.

Related:


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@bluwy bluwy added p2-nice-to-have Not breaking anything but nice to have (priority) plugin: vue YAO Yet another option... labels Feb 11, 2022
packages/plugin-vue/src/template.ts Outdated Show resolved Hide resolved
@innocenzi
Copy link
Contributor Author

innocenzi commented Feb 28, 2022

I think this is strictly a plugin-vue concern and should not need a Vite option.

May I kindly ask this to be reconsidered? The main use case is to allow this option to be configured from plugins specific to server integrations, so the user do not have to worry about it.

If adding a configuration option is not viable, is there another way? Better heuristics to set template.transformAssetUrls.includeAbsolute to true? Maybe when server.origin is defined?

EDIT - I updated the PR to do that. The Vue plugin will include absolute URLs by default, unless server.origin is defined. I'm thinking we could use config.publicDir instead though: if it's set to false, disable resolving absolute URLs. Would it make more sense? I think it would be cleaner in the logical sense, but I'm unsure about the side-effects.

@innocenzi innocenzi requested a review from yyx990803 March 9, 2022 18:44
@patak-dev patak-dev added p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) and removed p2-nice-to-have Not breaking anything but nice to have (priority) labels May 6, 2022
@patak-dev
Copy link
Member

@innocenzi I added this PR back for discussion. Your idea about automatically detecting the use case is interesting, but I don't know if everyone who sets server.origin will need this.

While we discuss, would you remove the automatic detection logic so we can let users disable this by configuring the Vue plugin? Even if it isn't ideal from the user's POV, at least we are unblocking Vue users.

@innocenzi
Copy link
Contributor Author

Hey @patak-dev, thanks!

I reverted the commit with the automatic detection. I'm not sure if this was ideal either, I just wanted to try to move forward.

What do you think? Should we expose the configuration option, or should we try a better heuristic? I think exposing the option would be the best - but another idea, more complex though, would be to find a way to let plugins configure other plugins. That'd potentially be a big change though.

@patak-dev
Copy link
Member

I was thinking we could do this in two stages. Let's first go with Evan's suggestion that doesn't need any new option, and would at least let users configure this through the Vue plugin using template.transformAssetUrls.includeAbsolute
I know it isn't ideal from the integration POV, but at least we can unblock the use case. And then we can discuss in another PR how to do this automatically or to let other plugins set this one. For example, we could use a similar approach to the one we merged here #5454

@innocenzi
Copy link
Contributor Author

Ah, yes, my bad. I misunderstood what you said. I reverted all new option changes

@patak-dev patak-dev removed the YAO Yet another option... label May 6, 2022
@patak-dev patak-dev changed the title feat: add resolve.absoluteUrls option (fix #4836) fix(plugin-vue): allow overwriting template.transformAssetUrls.includeAbsolute (fix #4836) May 6, 2022
@patak-dev patak-dev merged commit e0fe200 into vitejs:main May 6, 2022
@innocenzi innocenzi deleted the fix/vue-absolute-urls branch May 6, 2022 12:27
patak-dev pushed a commit that referenced this pull request May 11, 2022
@patak-dev
Copy link
Member

@innocenzi @jessarcher [email protected] includes this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants