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): use server.origin when building base for transformAssetUrls #8077

Merged
merged 4 commits into from
May 15, 2022

Conversation

segevfiner
Copy link
Contributor

@segevfiner segevfiner commented May 9, 2022

When you use server.origin to add an origin prefix to paths in development, the assets plugins and plugin-vue transform the paths differently so that:

<script setup lang="ts">
import logo from './assets/logo.svg';
</script>

<template>
    <img alt="Vue logo" class="logo" src="./assets/logo.svg" width="125" height="125" />
    <img alt="Vue logo" class="logo" :src="logo" width="125" height="125" />
</template>

Transforms differently. The first img tag won't have the server.origin as a prefix while the second img tag will have it.

The origin prefix is added to the path here in:

const origin = config.server?.origin ?? ''
return origin + config.base + rtn.replace(/^\//, '')

So I simply add it in the same way when building the base for transformAssetUrls in plugin-vue.

@segevfiner segevfiner force-pushed the plugin-vue-use-server-origin branch from 21cade8 to a343315 Compare May 9, 2022 10:22
@patak-dev
Copy link
Member

Would you add a test case so we avoid regressions? @innocenzi could you help us check this one?

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 9, 2022
@segevfiner
Copy link
Contributor Author

Would you add a test case so we avoid regressions? @innocenzi could you help us check this one?

Added a test. Had to add another playground for it since this needs a modified vite.config.ts, let me know if this is fine.

@segevfiner
Copy link
Contributor Author

ERR_PNPM_LOCKFILE_MISSING_DEPENDENCY WAT...?! But I did let the lock file update... Well let's try removing some dependencies that aren't really needed from the new playground since I just copied that package.json and see how that goes...

@patak-dev
Copy link
Member

Thanks! Dont mind the failing test, it is a glitch.

We are trying to keep the number of playgrounds low. There is already a tailwind playground that has set up a server.origin and is using vue, it is a bit of a stretch but maybe we could add the test for this there temporally. I shouldn't have said this before, sorry about that (we may need to add something to CONTRIBUTING.md).

Another option would be to add origin to a more representative playground, like backend-integration 🤔
But you can leave the test as is for the moment, @innocenzi maybe you have a better idea for the best playground where to test origin + vue

@segevfiner
Copy link
Contributor Author

The failure looks like some kind of flake: error while executing cli command "/Users/runner/work/vite/vite/packages/vite/bin/vite.js --port 9511 --strict-port": failed to start within 3000ms

@innocenzi
Copy link
Contributor

innocenzi commented May 10, 2022

@patak-dev I think everything related to server.origin can live in the backend-integration playground, that is what makes the most sense to me

I can also confirm this issue (related to the recently closed PR of mine btw). That being said, the fix doesn't work on my end:

Actual

<img alt="Vue logo" class="logo" src="https://captainjet.test:3000/preset.svg" width="125" height="125">

Expected

<img alt="Vue logo" class="logo" src="https://captainjet.test:3000/resources/vue/pages/preset.svg" width="125" height="125">

Using a relative path in the template doesn't seem to resolve relatively to the asset with that PR

@segevfiner
Copy link
Contributor Author

I will see if I can move it there. There is some sensitivity with trailing slashes there, it's hard for me to tell what went wrong on your end from the information you posted so it would help if you can figure it out on your end or supply some reproduction/test that shows where it still doesn't work.

@innocenzi
Copy link
Contributor

innocenzi commented May 10, 2022

My apologies: I tested it wrong (I copy-pasted the fix without updating the imports modified by the bundler).

I, in fact, had to add parenthesis to the null coalescing operator to make it work:

assetUrlOptions = {
	base: (options.devServer.config.server?.origin ?? '')
		+ options.devServer.config.base 
		+ slash(import_path3.default.relative(options.root, import_path3.default.dirname(filename)))
};

@segevfiner
Copy link
Contributor Author

segevfiner commented May 10, 2022

@patak-dev I think everything related to server.origin can live in the backend-integration playground, that is what makes the most sense to me

I can also confirm this issue (related to the recently closed PR of mine btw). That being said, the fix doesn't work on my end:

Actual

<img alt="Vue logo" class="logo" src="https://captainjet.test:3000/preset.svg" width="125" height="125">

Expected

<img alt="Vue logo" class="logo" src="https://captainjet.test:3000/resources/vue/pages/preset.svg" width="125" height="125">

Using a relative path in the template doesn't seem to resolve relatively to the asset with that PR

Sticking it in "backend-integration" is gonna mess it up a bit since there are assets in the existing html there which won't render after this as I'm setting it to a bogus server.origin. @innocenzi

@segevfiner
Copy link
Contributor Author

@innocenzi So any ideas? Should we still move the tests into backend-integration and if so, what should I set server.origin and base there? If we do stick with moving it there, I will have to also fix any tests that break due to this change.

@innocenzi
Copy link
Contributor

Honestly not sure. Seems like too much trouble to make it work there. @patak-dev is it fine if we keep this new playground for now?

@bluwy bluwy changed the title fix: Use server.origin when building the base for transformAssetUrls fix(plugin-vue): use server.origin when building base for transformAssetUrls May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev server configuration origin is ignored in Vue template asset URLs
5 participants