[stevematney]

This avoids a crash when there are css-only entries in a non-splitting build.

Existing tests cover the main use-cases. I think those are enough, but if desired, I can add a new playground to cover the above case.

[sapphi-red]

Would you provide a reproduction? or explain what the crash is and how this PR fixes it?

[stevematney]

@sapphi-red I've added a playground which sets up the conditions of the crash I was seeing in our downstream build, where we have an entry which is something like main.scss (or style-only.pcss in the playground). The problem was basically that the emptyJsPlaceholder didn't get added to the cssEntriesMap for css-only entries, so it would crash when trying to remove that placeholder with the following error (which I was able to replicate in the playground):

[plugin vite:css-post]
TypeError: Cannot read properties of undefined (reading 'referenceId')
    at file:///C:/.../projects/vite/packages/vite/dist/node/chunks/node.js:22859:109
    at Array.forEach (<anonymous>)
    at PluginContextImpl.generateBundle (file:///C:/.../projects/vite/packages/vite/dist/node/chunks/node.js:22855:23)
    at async plugin (file:///C:/.../projects/vite/node_modules/.pnpm/rolldown@1.2.0/node_modules/rolldown/dist/shared/bindingify-input-options-_ppP73I9.mjs:1707:4)
    at async plugin.<computed> (file:///C:/.../projects/vite/node_modules/.pnpm/rolldown@1.2.0/node_modules/rolldown/dist/shared/bindingify-input-options-_ppP73I9.mjs:1959:12)
    at aggregateBindingErrorsIntoJsError (file:///C:/.../projects/vite/node_modules/.pnpm/rolldown@1.2.0/node_modules/rolldown/dist/shared/error-DFGNOCle.mjs:48:18)
    at unwrapBindingResult (file:///C:/.../projects/vite/node_modules/.pnpm/rolldown@1.2.0/node_modules/rolldown/dist/shared/error-DFGNOCle.mjs:18:128)
    at #build (file:///C:/.../projects/vite/node_modules/.pnpm/rolldown@1.2.0/node_modules/rolldown/dist/shared/rolldown-build-CdF8HAHX.mjs:3276:34)
    at async buildEnvironment (file:///C:/.../projects/vite/packages/vite/dist/node/chunks/node.js:33669:66)
    at async Object.build (file:///C:/.../projects/vite/packages/vite/dist/node/chunks/node.js:34091:19)
    at async Object.buildApp (file:///C:/.../projects/vite/packages/vite/dist/node/chunks/node.js:34088:153)
    at async CAC.<anonymous> (file:///C:/.../projects/vite/packages/vite/dist/node/cli.js:776:3) {
  errors: [Getter/Setter]
}

This fixes it by ensuring that cssEntriesMap is properly populated whenever CSS for an entry chunk is output.

[sapphi-red]

As the error for .css describes, it's not expected to pass CSS files (including files generating.pcss) as an entrypoint when build.cssCodeSplit is set to false.

Instead of this configuration, if you add import './style-only.pcss' in index.js, you'll get the same behavior. What is the reason you are using this configuration?

[stevematney]

I think the main reason for it is that it's been that way for some time. We're building a component library with a separate CSS export, but we're not exporting our html. It's a little counter-intuitive that setting up the import in your html would be how you're expected to generate CSS package exports. I do have a couple of questions on that:

1. Will the CSS imported via html still be caught and generated in lib mode?
2. These changes treat the addition to cssEntriesMap a bit more consistently throughout the cssPostPlugin code. Is there value in having that consistency even if the originating cause is because of misconfiguration?

[sapphi-red]

1. Will the CSS imported via html still be caught and generated in lib mode?

It will only be included if you specify the HTML as an entrypoint. But I don't think that's the case. You need to import it so it's part of the module graph (i.e. reachable from the entrypoint).

2. These changes treat the addition to cssEntriesMap a bit more consistently throughout the cssPostPlugin code. Is there value in having that consistency even if the originating cause is because of misconfiguration?

If the code is simpler, I think we can merge it. But I'm not sure about the consistency for a unsupported functionality.

[stevematney]

If the code is simpler, I think we can merge it. But I'm not sure about the consistency for a unsupported functionality.

I don't know if I would say it's simpler, necessarily. It guarantees expectations about cssEntriesMap in later logic, and consolidates emitting an entry file with populating that map. So in this small way it's a little bit easier to reason about. In terms of complexity vs simplicity, it adds some redirection and a little bit more cognitive overhead when reading the code, so it's slightly more complex that way.

I'm not sure about the consistency for a unsupported functionality.

I'm also a bit stuck on this. It does make scss (and pcss) defined as their own entries work, but if that is an anti-pattern as far as vite is concerned, it seems like the early error condition should interrupt for those as well as regular css entries. I'm very new to the internal vite domain, but is there a way it could throw the error if the expected output for the entry is CSS rather than just checking the file extension?

[sapphi-red]

is there a way it could throw the error if the expected output for the entry is CSS rather than just checking the file extension?

It's a bit tricky. I think for now changing this if to use CSS_LANGS_RE would be better.

vite/packages/vite/src/node/build.ts

Line 640 in ef02435

if (inputs.some((input) => input.endsWith('.css'))) {

[stevematney]

I have made that update. I'm still not sure if you would like to keep the changes in css.ts. They do make some things somewhat "safer" than they were previously, but the issues those changes were intended to address can't be reproduced with the change to build.ts, and the changes aren't absolutely necessary.

The one thing I'm not sure about is if the manifest ends up with the correct CSS info in the final fall-through case without these changes. I think these changes ensure that because the emitted CSS gets added to cssEntriesMap. This is a case I'm not certain how to replicate and test, but if it is solving for that, it would be in this code (css.ts lines 1096-1121)

[stevematney]

@sapphi-red with regard to this comment, I did some research and found that the effect on the manifest was not more correct than it was previously, so I've removed the majority of the changes from css.ts.

The changes left in there are one ensuring that CSS entries created by plugins fail in the same way as those provided to entry directly, and a change accounting for empty CSS files when not in split mode. In split mode, a file is emitted for these chunks, so this change aligns the two modes in terms of their output. (A CSS file is emitted in both cases.)

If that's not desired, it's really easy to pull out, as it sits in its own commit, which I can remove if needed.