feat(rollup-plugin): warn stylesheet import for CSS extensibility

[rui-rayqiu]

Details

This PR updates rollup-plugin on stylesheet imports so that if a component imports an non-existing CSS file it will throw an error log a warning message, while keep the current behavior treating any non-existing CSS file as empty string (compiled to export default undefined).

The previous behavior can be problematic because the new CSS extensibility feature (a.k.a. programmatic style injection) allows users to override a component's style by programmatically importing a stylesheet module from JavaScript using an import statement. We should enforce that the imported CSS file must exist and if not the compilation should fail instead of treating it as an empty string so that developers can be informed the issue during build time. We should warn developers that the imported CSS file does not exist.

More details can be found in the LWC stylesheets modules section of the RFC.

The RFC proposes to use a new query parameter optional=true to indicate that an imported CSS is optional as following example.

// Required LWC stylesheet import. Fails during compilation when missing.
import styles from "./styles.css";
// Optional LWC stylesheet import. Resolve to an empty stylesheet if missing.
import styles from "./styles.css?optional=true";

This can be achieved by changing the template compiler to include the new optional query param in the implicit stylesheet imports. However, during implementation I find that it might be tricky for the case where component JS file also imports the same implicit stylesheet as following.

// app.js
import { LightningElement } from 'lwc';
import block from './app.css';
export default class App extends LightningElement {
    static blockStyle = block;
    // ...
}

Note that this component app also imports its implicit CSS app.css and its compiled HTML file will have import _implicitStylesheets from "./app.css?optional=true"; thus the rollup plugin needs to handle both app.css and app.css?optional=true properly to make sure it is the exact same file. And it can be confusing as in HTML it is marked as optional but in JS its marked as required. This case can be special handled but it could be slightly more complicated and could cause confusion in the future.

Instead I propose to simply check if a CSS import if implicit similar to how lwc-plaform does the check. Although this can be less explicit, this approach has some benefits:

• This will not involve any compiler changes so it is less impactful for downstream projects.
• The logic of CSS module resolving is still in one place inside rollup plugin.
• It better aligns with how lwc-platform or lwr where they do the similar check.
• Avoid adding too many query params as there is already one for scoped.
• It would make it easier in the future to refactor common module resolution for different projects.

Does this pull request introduce a breaking change?

• ✅ No, it does not introduce a breaking change.

Does this pull request introduce an observable change?

• ⚠️ Yes, it does include an observable change.
• Update: after resolving the PR comment, this should not have any observable changes.

This would be an observable change if existing components are importing a non-existing CSS module. But I think this should be a rare case and if it happens it is more likely an unnoticed error by component author and should be fixed by removing the import of non-existing CSS module.

GUS work item

W-10183295

[nolanlawson]

I like the simplicity of this implementation. It's more elegant IMO than the ?optional=true approach. However I am concerned about the potentially breaking change, so I would advise the following:

1. Rather than introduce a breaking change, let's keep the existing functionality
2. but let's log a warning for the case you identify, where a non-implicitly-imported CSS file doesn't exist (and potentially report that warning somewhere)

Otherwise I'm afraid we will run into breakages in practice. An example of this would be:

1. Customer writes import stylesheet from './stylesheet.css'
2. Deletes stylesheet.css later, forgets to remove the import
3. Everything works
4. Boom, it breaks after this PR

[nolanlawson]

I suppose we could remove the if and just always set absPath = filename. If scoped is false, then the two strings should be equal.

[rui-rayqiu]

Yes, that's correct. I've removed it.

[nolanlawson]

One nice thing about this PR is that we are no longer actually reading from the filesystem – we are just doing path string comparisons.

Also we don't have to generate our own compiler error here – I assume because Rollup will do it for us?

[rui-rayqiu]

Great advice, I've switched to use this.warn() instead.

[nolanlawson]

BTW it looks like there is already a this.warn() API we can use, which I assume consumers can hook into with onwarn.

[nolanlawson]

Suggested change

	it('should throw an error when import stylesheet file is missing', async () => {
	it('should emit a warning when import stylesheet file is missing', async () => {

[nolanlawson]

Can we check that the code is still compiled successfully here?

[nolanlawson]

Suggested change

	`The imported CSS file ${id} does not exist: Importing it as undefined.`
	`The imported CSS file ${id} does not exist: Importing it as undefined. ` +
	`This behavior may be removed in a future version of LWC. Please avoid importing a ` +
	`CSS file that does not exist.`

[nolanlawson]

LGTM! Some minor nitpicks.