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: don't inject CSS sourcemap for direct requests #13115

Merged
merged 4 commits into from May 15, 2023

Conversation

ArnaudBarre
Copy link
Member

@ArnaudBarre ArnaudBarre commented May 7, 2023

Introduced in #8094

You can this this kind of output in the css-sourcemap playground
Screenshot 2023-05-07 at 21 54 38

The default linked.css should not have sourcemap because there is no transformation happening, but the one with import gets correct source map

I don't know exactly why, but the plugin container will inject sourcemap in the case of direct request but not proxy request.

@ArnaudBarre ArnaudBarre added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 7, 2023
@ArnaudBarre ArnaudBarre self-assigned this May 7, 2023
@stackblitz
Copy link

stackblitz bot commented May 7, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

playground/css-sourcemap/__tests__/css-sourcemap.spec.ts Outdated Show resolved Hide resolved
return await getContentWithSourcemap(css)
return htmlProxyRE.test(id) ? await getContentWithSourcemap(css) : css
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should generate the sourcemap in the htmlInlineProxyPlugin.


Then, we can just run return null in this line I guess. (The plugin container will add the sourcemap if needed)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure to understand. I've read part of the code of the plugin and I don't understand where is the pipeline called. I've the impression that the cache map is set with the tag content directly without transformation and then on this line we return this content

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, my bad. I was misunderstanding the code. I pushed the code I had in my mind.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok I didn't saw that call to the pluginContainer and just saw the load part.
I have no opinion on where this code 🤷‍♂️

@sapphi-red
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented May 13, 2023

📝 Ran ecosystem CI: Open

suite result
astro ✅ success
histoire ✅ success
iles ✅ success
ladle ✅ success
laravel ✅ success
marko ✅ success
nuxt ❌ failure
previewjs ✅ success
qwik ✅ success
rakkas ✅ success
sveltekit ✅ success
unocss ✅ success
vite-plugin-ssr ✅ success
vite-plugin-react ✅ success
vite-plugin-react-pages ✅ success
vite-plugin-react-swc ✅ success
vite-plugin-svelte ✅ success
vite-plugin-vue ✅ success
vite-setup-catalogue ✅ success
vitepress ✅ success
vitest ✅ success

@sapphi-red
Copy link
Member

nuxt is failing with main branch too 👍

@patak-dev
Copy link
Member

For reference, @danielroe said the issue in Nuxt is happening due to a new test they updated. We already did a Vite patch with this issue present. Let's merge this PR too for the next patch.

@patak-dev patak-dev merged commit 7d80a47 into main May 15, 2023
13 checks passed
@patak-dev patak-dev deleted the fix-css-sourcemap-linked branch May 15, 2023 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css 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.

None yet

3 participants