Skip to content

fix: normalise css paths in manifest on windows (fixes #9295)#9353

Merged
patak-cat merged 4 commits intovitejs:mainfrom
timacdonald:css-asset-normalisation-on-windows
Jul 25, 2022
Merged

fix: normalise css paths in manifest on windows (fixes #9295)#9353
patak-cat merged 4 commits intovitejs:mainfrom
timacdonald:css-asset-normalisation-on-windows

Conversation

@timacdonald
Copy link
Contributor

@timacdonald timacdonald commented Jul 25, 2022 •
edited by sapphi-red
Loading

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.

This was referenced Jul 25, 2022
Closed
Closed
timacdonald
timacdonald commented Jul 25, 2022
View reviewed changes
packages/vite/src/node/plugins/manifest.ts
src: normalizePath(chunk.name!)
}

if (cssEntryFiles.has(chunk.name!)) manifestChunk.isEntry = true
Copy link
Contributor Author

@timacdonald timacdonald Jul 25, 2022

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

@sapphi-red sapphi-red Jul 25, 2022 •
edited
Loading

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

Copy link
Contributor Author

@timacdonald timacdonald Jul 25, 2022

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-cat patak-cat 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

timacdonald commented Jul 25, 2022

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

@patak-cat
Copy link
Member

patak-cat commented Jul 25, 2022

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

@timacdonald
Copy link
Contributor Author

timacdonald commented Jul 25, 2022

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 !

sapphi-red added a commit to aleen42/vite that referenced this pull request Jun 13, 2023
@sapphi-red
test: restore test for vitejs#9353
795e054
@sapphi-red sapphi-red mentioned this pull request Jun 13, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sapphi-red sapphi-red sapphi-red left review comments

@patak-cat patak-cat patak-cat approved these changes

Assignees

No one assigned

Labels

feat: css p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) windows only

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

CSS file paths in Vite 3 manifest broken on Windows

4 participants

@timacdonald @patak-cat @rachids @sapphi-red