fix: pcss in manifest.json saved as css (fixed #12009)

[faga295]

closed #12009

Description

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.
• Read the Pull Request Guidelines and follow the PR Title 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.

[bluwy]

/ecosystem-ci run

[bluwy]

Hmm vite-plugin-ssr seems to be consistently failing with this pr. @brillout maybe you have some ideas why it's happening here. (Note: vite-setup-catalogue is currently failing in main too)

[brillout]

VPS expects all style entries to end with .css, AFAICT rightful so. At least, I certainly don't expect PostCSS files to exist in dist/client/. Therefore I'd argue that all style files should end with .css.

[bluwy]

IIUC the output would still be .css. This PR fixes the case when the src in the manifest file points wrongly to, for example, src/style.css instead of src/style.pcss, where the latter is the actual existing file. Is vite-plugin-ssr using the src for something?

[brillout]

VPS doesn't use the src property, so it seems like this PR has some side effects? @faga295

[faga295]

Probably, maybe the assets originName has other uses

[brillout]

@faga295 I would $ diff manifest.json pre/post-PR and see what changes. Chances that you'll see the unwanted side effects right away. (VPS can't find assets post-PR.)

[faga295]

nice~this will help a lot

[brillout]

I think any project will surface the unwanted diff, probably doesn’t have to be a VPS one

…

On Tue 14. Feb 2023 at 17:59, faga ***@***.***> wrote: nice~this will help a lot — Reply to this email directly, view it on GitHub <#12030 (comment)>, or unsubscribe <https:/notifications/unsubscribe-auth/AAHVQRRGXTS6DQJIS6JL2UDWXO2VXANCNFSM6AAAAAAUZIHGJU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[faga295]

Seems the originalName of assets chunk only used in generating manifest.json.I really can't think of any other side effects.

[bluwy]

@brillout I'm pretty sure this PR doesn't introduce any unwanted change in the manifest, and only changes what's outlined at #12009 (comment). The originalName change in this PR is only used in a few places. I'll run vite-plugin-ssr's test suite again just in case, but this PR looks safe to me.

[bluwy]

/ecosystem-ci run vite-plugin-ssr

[bluwy]

Looks like it's still failing at https:/brillout/vite-plugin-ssr/blob/e0e96a570a2cc2ded9041fe40d4c68d0a99c7edb/vite-plugin-ssr/node/runtime/renderPage/getPageAssets/getManifestEntry.ts#L44, seems like it's indexing using the manifestKeyMap but this PR makes the index undefined? The getManifestKeyMap doesn't look out of the ordinary to me.

[brillout]

Side effect: brillout/vite-pr-12030-reprod@9e6893d#diff-3b0435c9efe073e8ab8263bbad459657ba8d6826e4a2e5079b67438cb535a466.

The only diff causing this side effect is this PR: brillout/vite-pr-12030-reprod@f0aec13#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519.

[brillout]

(@bluwy FYI brillout@5af52d1.)

[faga295]

Indeed, I debug it and I realized that cssAssetName is not always a cssRequest, such as index.html, so the originalName should be cssFileName instead of cssAssetName

[bluwy]

Thanks for helping to check that 🙂 Didn't expect a html file in the CSS plugin. I'll tweak the code a bit and run ecosystem-ci again.

[bluwy]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

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

[faga295]

Seems it still has some problems.

[faga295]

I think we should discuss the standard of the src of manifest.json, because sometimes the ?used query(maybe more other query) will occur in src, I think it's not a expect result.

this is a snapshot of manifest.json built in main branch

[faga295]

the simplest solution of this issue is add .pcss, .postcss, sss based on ispreprocessor, but not include css.