-
-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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: normalise css paths in manifest on windows (fixes #9295) #9353
fix: normalise css paths in manifest on windows (fixes #9295) #9353
Conversation
@@ -103,7 +103,7 @@ export function manifestPlugin(config: ResolvedConfig): Plugin { | |||
function createAsset(chunk: OutputAsset): ManifestChunk { | |||
const manifestChunk: ManifestChunk = { | |||
file: chunk.fileName, | |||
src: chunk.name | |||
src: normalizePath(chunk.name!) | |||
} | |||
|
|||
if (cssEntryFiles.has(chunk.name!)) manifestChunk.isEntry = true |
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.
I'm unsure if this reference to chunk.name
also needs to be normalised.
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.
Thank you for the PR! This PR works 👍
I think it is better to normalize the name here. (cssFileName
)
vite/packages/vite/src/node/plugins/css.ts
Lines 536 to 545 in ccb3449
const cssFileName = ensureFileExt(cssAssetName, '.css') | |
if (chunk.isEntry && isPureCssChunk) cssEntryFiles.add(cssAssetName) | |
chunkCSS = resolveAssetUrlsInCss(chunkCSS, cssAssetName) | |
chunkCSS = await finalizeCss(chunkCSS, true, config) | |
// emit corresponding css file | |
const fileHandle = this.emitFile({ | |
name: isPreProcessor(lang) ? cssAssetName : cssFileName, |
If you don't mind, I will push some changes since I'm using windows. 🙂
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.
Of course not! Would love any input and help with this one.
Thank you so much for taking a look at this and helping get it across the line. Really appreciate your time. |
How do you see the reception so far in the Laravel community of Vite as the default @timacdonald? |
Seems really positive. Lots of people really happy with the speed improvements they are seeing and a lot of interesting things coming out to improve the DX in the backend framework space. |
It is night and day ! |
Description
Fixes: #9295
We have had several reports in the Laravel official plugin repo that the Vite 3 CSS paths in the manifest are not being normalised as expected on Windows. We do not modify the manifest generation in any way in our plugin.
e.g. laravel/vite-plugin#101
There are also reports on the vite issue tracker for this: #9295
npm run build
may result in the following manifest:Notice the backslashes.
This PR proposes that we run the path through
normalisePath
before writing the manifest. This is what we did to fix this issue in the official Laravel plugin when we were doing this manually in Vite 2.Additional context
I do not have direct access to a Windows machine, so I need this to be verified by Windows users!!
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).