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: normalise css paths in manifest on windows (fixes #9295) #9353

Merged
merged 4 commits into from Jul 25, 2022

Conversation

timacdonald
Copy link
Contributor

@timacdonald timacdonald commented Jul 25, 2022

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:

{
  "resources/js/app.js": {
    "file": "assets/app.ab93cf8a.js",
    "src": "resources/js/app.js",
    "isEntry": true
  },
  "resources/css\\app.css": {
    "file": "assets/app.1776c6d9.css",
    "src": "resources/css\\app.css"
  }
}

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?

  • 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.

@timacdonald timacdonald force-pushed the css-asset-normalisation-on-windows branch from c6ec5b2 to 2caa3d7 Compare July 25, 2022 05:13
@@ -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
Copy link
Contributor Author

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.

Copy link
Member

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)

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. 🙂

Copy link
Contributor Author

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.

@sapphi-red sapphi-red added windows only feat: css p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels Jul 25, 2022
@patak-dev patak-dev merged commit 13e6450 into vitejs:main Jul 25, 2022
@timacdonald timacdonald deleted the css-asset-normalisation-on-windows branch July 25, 2022 20:44
@timacdonald
Copy link
Contributor Author

Thank you so much for taking a look at this and helping get it across the line. Really appreciate your time.

@patak-dev
Copy link
Member

How do you see the reception so far in the Laravel community of Vite as the default @timacdonald?

@timacdonald
Copy link
Contributor Author

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.

@rachids
Copy link

rachids commented Jul 26, 2022

It is night and day !
Thanks to all of you !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: css p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) windows only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSS file paths in Vite 3 manifest broken on Windows
4 participants