Skip to content

Commit

Permalink
fix(legacy): fix conflict with the modern build on css emitting (#6584)
Browse files Browse the repository at this point in the history
Fixes #3296
Supersedes #3317

The asset emitting conflict may also exist for other types of assets,
but let's fix the CSS one first.

The conflict here is due to the `hasEmitted` flag that was originally
intended to avoid duplicated CSS for multiple output formats
6bce108#diff-2cfbd4f4d8c32727cd8e1a561cffbde0b384a3ce0789340440e144f9d64c10f6R262-R263

When the legacy plugin is used, the flag was set to `true` for the
emitted CSS of the legacy bundle.
But the legacy plugin would remove all its emitted assets later to avoid
duplication.
So this logic results in no CSS to be actually emitted.

In this PR, I used a `__vite_skip_asset_emit__` flag to prevent the
CSS `generateBundle` from executing for the legacy build.
If other asset emitting plugins encounter similar issues, this flag
can be reused.
  • Loading branch information
haoqunjiang authored Jan 28, 2022
1 parent 8338e26 commit f48255e
Show file tree
Hide file tree
Showing 6 changed files with 30 additions and 1 deletion.
12 changes: 11 additions & 1 deletion packages/playground/legacy/__tests__/legacy.spec.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import {
listAssets,
findAssetFile,
isBuild,
readManifest,
untilUpdated
untilUpdated,
getColor
} from '../../testUtils'

test('should work', async () => {
Expand Down Expand Up @@ -50,6 +52,10 @@ test('generates assets', async () => {
)
})

test('correctly emits styles', async () => {
expect(await getColor('#app')).toBe('red')
})

if (isBuild) {
test('should generate correct manifest', async () => {
const manifest = readManifest()
Expand All @@ -73,4 +79,8 @@ if (isBuild) {
expect(findAssetFile(/index\./)).not.toMatch(terserPatt)
expect(findAssetFile(/polyfills-legacy/)).toMatch(terserPatt)
})

test('should emit css file', async () => {
expect(listAssets().some((filename) => filename.endsWith('.css')))
})
}
2 changes: 2 additions & 0 deletions packages/playground/legacy/main.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import './style.css'

async function run() {
const { fn } = await import('./async.js')
fn()
Expand Down
3 changes: 3 additions & 0 deletions packages/playground/legacy/style.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
#app {
color: red;
}
1 change: 1 addition & 0 deletions packages/playground/legacy/vite.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ module.exports = {
],

build: {
cssCodeSplit: false,
manifest: true,
rollupOptions: {
output: {
Expand Down
8 changes: 8 additions & 0 deletions packages/plugin-legacy/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,14 @@ function viteLegacyPlugin(options = {}) {
// entirely.
opts.__vite_force_terser__ = true

// @ts-ignore
// In the `generateBundle` hook,
// we'll delete the assets from the legacy bundle to avoid emitting duplicate assets.
// But that's still a waste of computing resource.
// So we add this flag to avoid emitting the asset in the first place whenever possible.
opts.__vite_skip_asset_emit__ = true

// @ts-ignore avoid emitting assets for legacy bundle
const needPolyfills =
options.polyfills !== false && !Array.isArray(options.polyfills)

Expand Down
5 changes: 5 additions & 0 deletions packages/vite/src/node/plugins/css.ts
Original file line number Diff line number Diff line change
Expand Up @@ -468,6 +468,11 @@ export function cssPostPlugin(config: ResolvedConfig): Plugin {
},

async generateBundle(opts, bundle) {
// @ts-ignore asset emits are skipped in legacy bundle
if (opts.__vite_skip_asset_emit__) {
return
}

// remove empty css chunks and their imports
if (pureCssChunks.size) {
const emptyChunkFiles = [...pureCssChunks]
Expand Down

0 comments on commit f48255e

Please sign in to comment.