fix(css): correctly set manifest source name and emit CSS file #14945

sapphi-red · 2023-11-10T14:44:55Z

Description

319e985: just simplifies the logic, it should be equivalent
b16ed8e: separate the process of generating the filename and the process of getting the original filename
c2a750d: generate the filename based on the chunk name
f1b6f67: fixes the test because #11578 changed the original filename incorrectly and the test was relying that fact

close #8057
fixes #12009
fixes #12072
fixes #14943
close #12030

refs #6649
refs #11578

Additional context

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, especially the Pull Request Guidelines.
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).
Update the corresponding documentation if needed.
Ideally, include relevant tests that fail without this PR but pass with it.

sapphi-red · 2023-11-10T15:17:10Z

/ecosystem-ci run

bluwy · 2023-11-10T16:06:33Z

I don't think this is a bad change, but we're quite close to the release now to make this change. I also saw the similar issues like #11413 and figured it's better to wait till next major.

If we do favour in merging this, I'd be comfortable only releasing stable on next Friday. I'd be bumped that we have to delay but future me will probably appreciate present me making exceptions 😄

patak-dev · 2023-11-10T17:19:31Z

Nice work here @sapphi-red! I think it is worth trying to merge this in Vite 5 after reviewing why it is breaking SvelteKit. Delaying a few days the release would not only be good for this change, but to give a few more days to maintainers to squash the other reds, so I vote for that exception too @bluwy 🙏🏼

sapphi-red · 2023-11-11T03:43:13Z

The reason why SvelteKit test in vite-plugin-svelte failed was because the CSS chunk entry overrode the JS chunk entry. .svelte files maps to both JS chunk and CSS chunk. Before this PR, the original source file name of Foo.svelte's CSS chunks was output as Foo.css which is wrong. This PR fixes that to be Foo.svelte. That caused the conflict with the JS chunk to happen.

I wasn't able to add a test for this, because this depends on the order of bundle which is difficult to control.

.svelte-kit\output\server\.vite content

Before this PR (contains src/routes/+page.css that doesn't exist)

{
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/error.svelte": {
    "file": "entries/fallbacks/error.svelte.js",
    "imports": [
      "_ssr.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/error.svelte"
  },
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/layout.svelte": {
    "file": "entries/fallbacks/layout.svelte.js",
    "imports": [
      "_ssr.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/layout.svelte"
  },
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/server/index.js": {
    "file": "index.js",
    "imports": [
      "_internal.js",
      "_index.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/server/index.js"
  },
  ".svelte-kit/generated/server/internal.js": {
    "file": "internal.js",
    "imports": [
      "_internal.js"
    ],
    "isEntry": true,
    "src": ".svelte-kit/generated/server/internal.js"
  },
  "_index.js": {
    "file": "chunks/index.js",
    "imports": [
      "_ssr.js"
    ]
  },
  "_internal.js": {
    "file": "chunks/internal.js",
    "imports": [
      "_ssr.js"
    ]
  },
  "_ssr.js": {
    "file": "chunks/ssr.js"
  },
  "src/routes/+page.css": {
    "file": "_app/immutable/assets/_page.1A-_w4me.css",
    "src": "src/routes/+page.css"
  },
  "src/routes/+page.js": {
    "file": "entries/pages/_page.js",
    "isEntry": true,
    "src": "src/routes/+page.js"
  },
  "src/routes/+page.svelte": {
    "css": [
      "_app/immutable/assets/_page.1A-_w4me.css"
    ],
    "file": "entries/pages/_page.svelte.js",
    "imports": [
      "_ssr.js",
      "_index.js"
    ],
    "isEntry": true,
    "src": "src/routes/+page.svelte"
  }
}

this PR without 1376a34 (src/routes/+page.svelte points the CSS file)

{
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/error.svelte": {
    "file": "entries/fallbacks/error.svelte.js",
    "imports": [
      "_ssr.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/error.svelte"
  },
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/layout.svelte": {
    "file": "entries/fallbacks/layout.svelte.js",
    "imports": [
      "_ssr.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/layout.svelte"
  },
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/server/index.js": {
    "file": "index.js",
    "imports": [
      "_internal.js",
      "_index.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/server/index.js"
  },
  ".svelte-kit/generated/server/internal.js": {
    "file": "internal.js",
    "imports": [
      "_internal.js"
    ],
    "isEntry": true,
    "src": ".svelte-kit/generated/server/internal.js"
  },
  "_index.js": {
    "file": "chunks/index.js",
    "imports": [
      "_ssr.js"
    ]
  },
  "_internal.js": {
    "file": "chunks/internal.js",
    "imports": [
      "_ssr.js"
    ]
  },
  "_ssr.js": {
    "file": "chunks/ssr.js"
  },
  "src/routes/+page.js": {
    "file": "entries/pages/_page.js",
    "isEntry": true,
    "src": "src/routes/+page.js"
  },
  "src/routes/+page.svelte": {
    "file": "_app/immutable/assets/entries/pages/_page.1A-_w4me.css",
    "src": "src/routes/+page.svelte"
  }
}

with 1376a34

{
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/error.svelte": {
    "file": "entries/fallbacks/error.svelte.js",
    "imports": [
      "_ssr.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/error.svelte"
  },
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/layout.svelte": {
    "file": "entries/fallbacks/layout.svelte.js",
    "imports": [
      "_ssr.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/components/layout.svelte"
  },
  "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/server/index.js": {
    "file": "index.js",
    "imports": [
      "_internal.js",
      "_index.js"
    ],
    "isEntry": true,
    "src": "../../../node_modules/.pnpm/@sveltejs+kit@1.27.3_svelte@4.2.1_vite@5.0.0-beta.17/node_modules/@sveltejs/kit/src/runtime/server/index.js"
  },
  ".svelte-kit/generated/server/internal.js": {
    "file": "internal.js",
    "imports": [
      "_internal.js"
    ],
    "isEntry": true,
    "src": ".svelte-kit/generated/server/internal.js"
  },
  "_index.js": {
    "file": "chunks/index.js",
    "imports": [
      "_ssr.js"
    ]
  },
  "_internal.js": {
    "file": "chunks/internal.js",
    "imports": [
      "_ssr.js"
    ]
  },
  "_ssr.js": {
    "file": "chunks/ssr.js"
  },
  "src/routes/+page.js": {
    "file": "entries/pages/_page.js",
    "isEntry": true,
    "src": "src/routes/+page.js"
  },
  "src/routes/+page.svelte": {
    "css": [
      "_app/immutable/assets/entries/pages/_page.1A-_w4me.css"
    ],
    "file": "entries/pages/_page.svelte.js",
    "imports": [
      "_ssr.js",
      "_index.js"
    ],
    "isEntry": true,
    "src": "src/routes/+page.svelte"
  }
}

sapphi-red · 2023-11-11T03:43:30Z

/ecosystem-ci run

vite-ecosystem-ci · 2023-11-11T04:14:29Z

📝 Ran ecosystem CI on 1376a34: Open

suite	result	latest scheduled
analogjs	❌ failure	❌ failure
astro	✅ success	✅ success
histoire	❌ failure	❌ failure
ladle	✅ success	✅ success
laravel	❌ failure	❌ failure
marko	✅ success	❌ failure
nuxt	❌ failure	❌ failure
nx	❌ failure	❌ failure
previewjs	✅ success	✅ success
qwik	❌ failure	❌ failure
rakkas	✅ success	✅ success
sveltekit	❌ failure	❌ failure
unocss	✅ success	✅ success
vike	❌ failure	❌ failure
vite-plugin-pwa	✅ success	✅ success
vite-plugin-react	✅ success	✅ success
vite-plugin-react-pages	❌ failure	❌ failure
vite-plugin-react-swc	✅ success	✅ success
vite-plugin-svelte	✅ success	✅ success
vite-plugin-vue	✅ success	✅ success
vite-setup-catalogue	✅ success	✅ success
vitepress	✅ success	✅ success
vitest	✅ success	❌ failure

bluwy

Thanks for the explanation for the v-p-s/SvelteKit fail. I was a bit concerned with the change, but the new manifest output does seem more correct 👍

Related: vitejs#14945 Fixes: vitejs#15161

sapphi-red added 3 commits November 10, 2023 23:15

refactor: simplify original name logic

319e985

fix: correctly set CSS originalFileName

b16ed8e

fix: correctly emit CSS file

c2a750d

sapphi-red added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) labels Nov 10, 2023

test: fix filename test and manifest windows path test

f1b6f67

sapphi-red mentioned this pull request Nov 10, 2023

revert: build assets with the entry name when it is an entry point (#11578) #14944

Closed

4 tasks

sapphi-red added 2 commits November 11, 2023 00:31

test: add case for vitejs#12009

61c19bf

test: add case for vitejs#12072

ed2be42

This comment was marked as outdated.

Sign in to view

fix: prioritize JS chunk in manifest

1376a34

patak-dev approved these changes Nov 11, 2023

View reviewed changes

bluwy approved these changes Nov 11, 2023

View reviewed changes

bluwy merged commit 28ccede into vitejs:main Nov 11, 2023
11 checks passed

sapphi-red deleted the fix/css-manifest-source-name branch November 11, 2023 15:21

bluwy mentioned this pull request Nov 12, 2023

Vite renaming CSS Modules filenames during build #13401

Closed

7 tasks

dominikg mentioned this pull request Nov 26, 2023

Regression in CSS output location #15141

Closed

7 tasks

This was referenced Nov 27, 2023

fix(css): use non-nested chunk name if facadeModule is not CSS file #15155

Merged

fix(css): render correct asset url when CSS chunk name is nested #15154

Merged

bluwy mentioned this pull request Nov 27, 2023

manifest.json does not contain css files imported by js entry points any more #15161

Closed

helhum added a commit to helhum/vite that referenced this pull request Nov 27, 2023

Reflect changes to manifest.json in migration.md

7048356

Related: vitejs#14945 Fixes: vitejs#15161

helhum mentioned this pull request Nov 27, 2023

docs: reflect changes to manifest.json in migration.md #15165

Merged

8 tasks

sapphi-red added a commit to helhum/vite that referenced this pull request Nov 28, 2023

docs: add vitejs#14945 to advanced section

40b3680

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(css): correctly set manifest source name and emit CSS file #14945

fix(css): correctly set manifest source name and emit CSS file #14945

sapphi-red commented Nov 10, 2023 •

edited

sapphi-red commented Nov 10, 2023

This comment was marked as outdated.

bluwy commented Nov 10, 2023

This comment was marked as outdated.

patak-dev commented Nov 10, 2023

sapphi-red commented Nov 11, 2023

sapphi-red commented Nov 11, 2023

vite-ecosystem-ci bot commented Nov 11, 2023

bluwy left a comment

fix(css): correctly set manifest source name and emit CSS file #14945

fix(css): correctly set manifest source name and emit CSS file #14945

Conversation

sapphi-red commented Nov 10, 2023 • edited

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

sapphi-red commented Nov 10, 2023

This comment was marked as outdated.

bluwy commented Nov 10, 2023

This comment was marked as outdated.

patak-dev commented Nov 10, 2023

sapphi-red commented Nov 11, 2023

sapphi-red commented Nov 11, 2023

vite-ecosystem-ci bot commented Nov 11, 2023

bluwy left a comment

Choose a reason for hiding this comment

sapphi-red commented Nov 10, 2023 •

edited