fix(plugin-vue): make cssm code tree shakeable

[fnlctrl]

Description

Currently plugin-vue compiles sfc css modules to assignment expressions, which makes them unable to be tree shaken for a component library.

Before:

const cssModules = {};
cssModules["$style"] = style;
cssModules["Foo"] = style2;

After this PR:

const cssModules = {
"$style": style,
"Foo": style2
};

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

[Niputi]

could you add a test for this?

[fnlctrl]

could you add a test for this?

Sure! Should I add it under packages/playground/vue/__tests__ ?

Ideally this should be a unit test and not a e2e test involving browsers, but packages/plugin-vue doesn't have its own __tests__..

[Niputi]

there's a small guide for adding tests https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md

[Niputi]

please move the test to the playground directory and rollback the dependency downgrade

[fnlctrl]

there's a small guide for adding tests https://github.com/vitejs/vite/blob/main/CONTRIBUTING.md

Thanks! I found the test to not be appropriate under packages/playground/vue/__tests__ as these tests rely on DOM state and there's no straightforward way to test function output. So I added packages/plugin-vue/__tests__/transform.spec.ts instead.

I also downgraded slash from 4.0.0 to 3.0.0 for the test to run, as jest can't handle 4.0.0's ESM export (jest doens't have native ESM support for now).

These two versions are basically just the same except for the export syntax, so I think it shouldn't be an issue.
https://unpkg.com/[email protected]/index.js
https://unpkg.com/[email protected]/index.js

please move the test to the playground directory and rollback the dependency downgrade

Oops, I replied too late after the commit. Please see my reasoning above.

[haoqunjiang]

Ideally this should be a unit test and not a e2e test involving browsers, but packages/plugin-vue doesn't have its own __tests__..

IMO, ideally, we need an E2E test, that checks :

1. the styles that are not meant to be tree-shaked should still be applied;
2. if (isBuild), check the output CSS file, to see if the unused styles are still there.

Because what this PR really fixes is the tree-shakability of the output, not demanding a specific shape of the generated code. Snapshot tests are easy to write but hard to maintain.

You can refer to

vite/packages/playground/assets/__tests__/assets.spec.ts

Lines 187 to 196 in 65cd44d

	test('from js import', async () => {
	const src = readFile('テスト-測試-white space.js')
	expect(await page.textContent('.unicode-url')).toMatch(
	isBuild
	? `data:application/javascript;base64,${Buffer.from(src).toString(
	'base64'
	)}`
	: `/foo/テスト-測試-white space.js`
	)
	})

as an example.

And we can avoid downgrading slash if we use E2E tests instead.

[fnlctrl]

Sorry if I didn't make the context clear enough. This intended for library mode only.

Scenario: For a component library exporting Button.vue and Input.vue, when building for the es target, both css module objects are included in the bundle. If the consumer only uses Button, Input's code should be completely tree-shaken, but the current shape of cssModules prevents that.

1. the styles that are not meant to be tree-shaked should still be applied;

This is already ensured by all existing e2e tests (if they were tree-shaked, none of the styles could have been applied and thus tests fail)

2. if (isBuild), check the output CSS file, to see if the unused styles are still there.

This is probably not applicable to library mode. This fix is trying to tree shake unused components's cssModules objects, not unused css modules within a component. Also, for normal mode, only components intended for actual use are imported from source, so tree shaking isn't much of a concern.

Because what this PR really fixes is the tree-shakability of the output, not demanding a specific shape of the generated code. Snapshot tests are easy to write but hard to maintain.

I will try to add a test for building a component library and then consuming that library, and check that the resulting bundle doesn't contain unused css modules code from the component library. This should be an e2e tests in the sense that the other end is developers consuming the package, not headless browsers.

---

---

---

Some unrelated rant I really want to get off my chest. My sincere apologies.

And we can avoid downgrading slash if we use E2E tests instead.

For slash:
I don't see why it should be upgraded to 4.0 in the first place.

I totally understand and support the package author's push for ES module adoption, by changing the current package to use esm export. This way people installing the package will find that it doesn't work by default, do some googling and find out how to set up ESM for their projects. Awareness for ESM is raised.

Great! Goal achieved - I now want to use ESM everywhere. But the fact is some critical infrastucture like jest just doesn't support ESM yet. OK, can't we just switch back to 3.0 now? slash is a very mature package with 3.0 published 3 years ago. 4.0 didn't change anything except for the export syntax. I don't foresee any potential change exclusive to the 4.x line in the future.

The bottom line is we should not let one package affect how we write tests. If some tests can be written as unit tests, they just don't have use headless browsers.

For E2E tests:
They're very very flaky to run locally, and the setup is extremely complicated. I still can't run the all the tests on my windows machine.

Some error excerpts (from a fresh clone + fresh pnpm i folder on the main branch):

Test Suites: 2 failed, 43 passed, 45 total
Tests:       10 failed, 294 passed, 304 total
Snapshots:   0 total
Time:        42.771 s, estimated 133 s

 FAIL  packages/playground/ssr-deps/__tests__/ssr-deps.spec.ts (39.662 s)
  ● msg should be encrypted

    thrown: "Exceeded timeout of 30000 ms for a hook.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      56 | }
      57 |
    > 58 | beforeAll(async () => {
         | ^
      59 |   const page = global.page
      60 |   if (!page) {
      61 |     return

      at Object.<anonymous> (scripts/jestPerTestSetup.ts:58:1)

  ● msg read by fs/promises

    thrown: "Exceeded timeout of 30000 ms for a hook.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      56 | }
      57 |
    > 58 | beforeAll(async () => {
         | ^
      59 |   const page = global.page
      60 |   if (!page) {
      61 |     return

      at Object.<anonymous> (scripts/jestPerTestSetup.ts:58:1)

  ● msg from primitive export

    thrown: "Exceeded timeout of 30000 ms for a hook.
    Use jest.setTimeout(newTimeout) to increase the timeout value, if this is a long-running test."

      56 | }
      57 |
    > 58 | beforeAll(async () => {
         | ^
      59 |   const page = global.page
      60 |   if (!page) {
      61 |     return

      at Object.<anonymous> (scripts/jestPerTestSetup.ts:58:1)
  ● Test suite failed to run

    listen EADDRINUSE: address already in use :::9530

      17 |   return new Promise((resolve, reject) => {
      18 |     try {
    > 19 |       const server = app.listen(port, () => {
         |                          ^
      20 |         resolve({
      21 |           // for test teardown
      22 |           async close() {

      at Function.listen (node_modules/.pnpm/[email protected]/node_modules/express/lib/application.js:618:24)
      at packages/playground/ssr-deps/__tests__/serve.js:19:26
      at serve (packages/playground/ssr-deps/__tests__/serve.js:17:10)
      at scripts/jestPerTestSetup.ts:87:20

I guess they fail because some race condition, but I don't want to waste a whole day trying to find out.

For simple tests like testing a function output, or the bundling result, we should probably just stick to plain old unit tests that doesn't rely on playwright.

For the port issue, I'm not sure why these some tests are using the same port. Maybe we migrate to get-port instead (the 5.1.1 version with cjs export, of course🤣)?

[haoqunjiang]

This intended for library mode only

Sorry for having misunderstood this PR.

In that case, I understand that the E2E test might be not easy to write so I'm fine without a test.
But the unit test is something of extra complexity that I personally don't wanna include.

---

For slash:

1. plugin-vue is bundled so ESM is a non-issue.
2. Always using the latest version can reduce the mental burden when we update the dependency version.
3. Yes, Jest doesn't support Node.js native ESM well. But that's Jest's problem. We might eventually migrate away from Jest one day. (People use Vite with native Node ES modules, too. It's something that we can't avoid. Even if we don't use them in the source code, we need native ES modules in a test case anyway)

For E2E tests:

I totally agree.
It's unfortunate that most of the team members don't use Windows machines for development, so we have to rely on the CI and community feedback to fix the windows issues. This definitely can be improved.

For the port issue, I'm not sure why these some tests are using the same port. Maybe we migrate to get-port instead (the 5.1.1 version with cjs export, of course🤣)?

Sounds good!