fix: avoid css leaking into emitted javascript #3402

ferdinando-ferreira · 2021-05-13T10:47:03Z

Description

Even with styles being emitted to their own .css files, their source code is present into the emitted .js files (both in regular and ssr modes) into a variable that is never used in the resulting source code.

It appears that this was added to prevent the css module from being "tree shaken" out of the compilation in this point of code so it can be used afterwards in the renderChunk hook.

vite/packages/vite/src/node/plugins/css.ts

Lines 166 to 175 in 4a955bc

    
           if (!code) { 
        
             if (config.build.cssCodeSplit) { 
        
               // If code-splitting CSS, inject a fake marker to avoid the module 
        
               // from being tree-shaken. This preserves the .css file as a 
        
               // module in the chunk's metadata so that we can retrieve them in 
        
               // renderChunk. 
        
               code += `${cssInjectionMarker}()\n` 
        
             } 
        
             code += `export default ${JSON.stringify(css)}` 
        
           }

Now, given that it doesn't appear to be used anywhere, it may not be necessary for the actual css code to be available in the variable, a simple "null" value would be enough.

Additional context

Fixes #3397.

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.

fixes vitejs#3397

Shinigami92

Is it also possible to add a test in the playgrounds to prevent regression?

Also looks like there are now failing tests 🤔
You can run these also locally with yarn test-build and yarn test-serve

packages/vite/src/node/plugins/css.ts

fixes vitejs#3397

ferdinando-ferreira · 2021-05-13T11:16:06Z

Also looks like there are now failing tests 🤔
You can run these also locally with yarn test-build and yarn test-serve

Will do and try to understand the reason for the failure

fixes vitejs#3397

ferdinando-ferreira · 2021-05-13T11:48:28Z

Also looks like there are now failing tests 🤔
You can run these also locally with yarn test-build and yarn test-serve

Thanks, I was not aware of these. The reason the test was failing was due to a test for an unrelated regression (for #132).

vite/packages/playground/resolve/index.html

Lines 159 to 163 in 30ff5a2

    
           // css entry 
        
           import css from 'normalize.css' 
        
           if (typeof css === 'string') { 
        
             text('.css', '[success] resolve package with css entry file') 
        
           }

It tested that a css was sucessfully included by verifying if the result was a string.

I changed the returned module from export default null to export default '' because there may be the assumption that importing a css file returns either an object (in case of css modules) or a string (otherwise).

Is it also possible to add a test in the playgrounds to prevent regression?

Yes, working on it.

test fixes vitejs#3397

ferdinando-ferreira · 2021-05-13T17:37:56Z

@Shinigami92 : done! Took advantage of the existing test for the plugin and added an extra test to ensure it's working properly for imports without variables.

Other than that the existing test proves it is working correctly by showing that the css source code is no longer being returned by the export. Here are the steps, from the root of the project

cd packages/playground/css/
yarn build
yarn serve

There will be a page served from http://localhost:5000/ containing the existing test and the new one, along with the now empty "imported __ string".

That will likely result in a massively reduced build size for entries with imported css across the board. In my project alone, the resulting dist/index.js went from 474K to 14K without change in functionality or behaviour.

$ du -b assets/client/dist/index.js old/client/dist/index.js
 14452  assets/client/dist/index.js
474629  old/client/dist/index.js

Please review and let me know if there is any required change.

Shinigami92 · 2021-05-13T18:00:27Z

Oh no 🙁
Seems there is a failing test

tests fixes vitejs#3397

ferdinando-ferreira · 2021-05-13T18:15:38Z

Seems there is a failing test

There was a typo in the test I added, it must be fixed now.

thecrypticace · 2021-05-30T00:42:58Z

This breaks production builds that import a css file to get the css text (I use this to style a vue component subtree that's mounted in the shadow DOM). Given that the text is still available in dev it was a bit surprising seeing the behavior difference.

Though, I'm in favor of removing css from the emitted js in prod bundles. It seems like #2148 (comment) could be a possible resolution to this problem.

)" fix for vitejs#3610 and partial fix for vitejs#2282 This reverts commit 65d333d

fix: avoid css leaking into emitted javascript

a8045ca

fixes vitejs#3397

Shinigami92 added the p3-minor-bug An edge case that only affects very specific usage (priority) label May 13, 2021

Shinigami92 reviewed May 13, 2021

View reviewed changes

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

fix: avoid css leaking into emitted javascript

55dd48f

fixes vitejs#3397

fix: avoid css leaking into emitted javascript

11066e5

fixes vitejs#3397

ferdinando-ferreira added 2 commits May 13, 2021 18:02

test: add text to the fix for vitejs#3397

3a6c10c

test fixes vitejs#3397

test: add text to the fix for vitejs#3397

97cba80

test fixes vitejs#3397

test: add text to the fix for vitejs#3397

b941318

tests fixes vitejs#3397

Shinigami92 approved these changes May 13, 2021

View reviewed changes

Shinigami92 requested review from antfu, nihalgonsalves and patak-dev May 13, 2021 18:32

nihalgonsalves approved these changes May 17, 2021

View reviewed changes

antfu merged commit 65d333d into vitejs:main May 23, 2021

anncwb mentioned this pull request Jun 1, 2021

Regression: can not get css text in the plugin #3610

Closed

6 tasks

ygj6 pushed a commit to ygj6/vite that referenced this pull request Jun 1, 2021

fix: avoid css leaking into emitted javascript (vitejs#3402)

dfccbf6

dominikg added a commit to dominikg/vite that referenced this pull request Jun 1, 2021

fix: Revert "fix: avoid css leaking into emitted javascript (vitejs#3402

cc9b7bf

)" fix for vitejs#3610 and partial fix for vitejs#2282 This reverts commit 65d333d

dominikg mentioned this pull request Jun 1, 2021

fix: revert avoid css leaking into emitted javascript (#3402) #3630

Merged

9 tasks

patak-dev pushed a commit that referenced this pull request Jun 2, 2021

fix: revert avoid css leaking into emitted javascript (#3402) (#3630)

91eb2a6

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: avoid css leaking into emitted javascript #3402

fix: avoid css leaking into emitted javascript #3402

ferdinando-ferreira commented May 13, 2021

Shinigami92 left a comment •

edited

Loading

ferdinando-ferreira commented May 13, 2021

ferdinando-ferreira commented May 13, 2021

ferdinando-ferreira commented May 13, 2021

Shinigami92 commented May 13, 2021

ferdinando-ferreira commented May 13, 2021 •

edited

Loading

thecrypticace commented May 30, 2021

	if (!code) {
	if (config.build.cssCodeSplit) {
	// If code-splitting CSS, inject a fake marker to avoid the module
	// from being tree-shaken. This preserves the .css file as a
	// module in the chunk's metadata so that we can retrieve them in
	// renderChunk.
	code += `${cssInjectionMarker}()\n`
	}
	code += `export default ${JSON.stringify(css)}`
	}

fix: avoid css leaking into emitted javascript #3402

fix: avoid css leaking into emitted javascript #3402

Conversation

ferdinando-ferreira commented May 13, 2021

Description

Additional context

What is the purpose of this pull request?

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

Shinigami92 left a comment • edited Loading

Choose a reason for hiding this comment

ferdinando-ferreira commented May 13, 2021

ferdinando-ferreira commented May 13, 2021

ferdinando-ferreira commented May 13, 2021

Shinigami92 commented May 13, 2021

ferdinando-ferreira commented May 13, 2021 • edited Loading

thecrypticace commented May 30, 2021

Shinigami92 left a comment •

edited

Loading

ferdinando-ferreira commented May 13, 2021 •

edited

Loading