-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: avoid splitting vendor chunk by default #6534
Changes from 3 commits
d0fda58
641e725
0a06f13
2306dad
41421c4
9e07062
a4d75bc
23440e9
060c1a3
df912c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
import type { Plugin } from '../plugin' | ||
import type { OutputOptions, GetManualChunk, GetModuleInfo } from 'rollup' | ||
import { isCSSRequest } from './css' | ||
|
||
// Use splitVendorChunkPlugin() to get the same manualChunks strategy as Vite 2.7 | ||
// We don't recommend using this strategy as a general solution moving forward | ||
|
||
// splitVendorChunk is a simple index/vendor strategy that was used in Vite | ||
// until v2.8. It is exposed to let people continue to use it in case it was | ||
// working well for their setups. | ||
// The cache needs to be reset on buildStart for watch mode to work correctly | ||
// Don't use this manualChunks strategy for ssr, lib mode, and 'umd' or 'iife' | ||
|
||
export class SplitVendorChunkCache { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class seems unnecessary. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know if we will need to expand the cache later. I wanted to expose the cache for the method, not a particular Map that is an implementation detail and may change |
||
cache: Map<string, boolean> | ||
constructor() { | ||
this.cache = new Map<string, boolean>() | ||
} | ||
reset() { | ||
this.cache = new Map<string, boolean>() | ||
} | ||
} | ||
|
||
export function splitVendorChunk({ | ||
cache = new SplitVendorChunkCache() | ||
}): GetManualChunk { | ||
patak-dev marked this conversation as resolved.
Show resolved
Hide resolved
patak-dev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return (id, { getModuleInfo }) => { | ||
if ( | ||
id.includes('node_modules') && | ||
!isCSSRequest(id) && | ||
staticImportedByEntry(id, getModuleInfo, cache.cache) | ||
) { | ||
return 'vendor' | ||
} | ||
} | ||
} | ||
|
||
function staticImportedByEntry( | ||
id: string, | ||
getModuleInfo: GetModuleInfo, | ||
cache: Map<string, boolean>, | ||
importStack: string[] = [] | ||
): boolean { | ||
if (cache.has(id)) { | ||
return cache.get(id) as boolean | ||
} | ||
if (importStack.includes(id)) { | ||
// circular deps! | ||
cache.set(id, false) | ||
return false | ||
} | ||
const mod = getModuleInfo(id) | ||
if (!mod) { | ||
cache.set(id, false) | ||
return false | ||
} | ||
|
||
if (mod.isEntry) { | ||
cache.set(id, true) | ||
return true | ||
} | ||
const someImporterIs = mod.importers.some((importer) => | ||
staticImportedByEntry( | ||
importer, | ||
getModuleInfo, | ||
cache, | ||
importStack.concat(id) | ||
) | ||
) | ||
cache.set(id, someImporterIs) | ||
return someImporterIs | ||
} | ||
|
||
export function splitVendorChunkPlugin(): Plugin { | ||
const cache = new SplitVendorChunkCache() | ||
return { | ||
name: 'vite:split-vendor-chunk', | ||
config(config) { | ||
const build = config.build ?? {} | ||
const format = (build.rollupOptions?.output as OutputOptions)?.format | ||
patak-dev marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if (!build.ssr && !build.lib && format !== 'umd' && format !== 'iife') { | ||
aleclarson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return { | ||
build: { | ||
rollupOptions: { | ||
manualChunks: splitVendorChunk({ cache }) | ||
} | ||
} | ||
} | ||
} | ||
}, | ||
buildStart() { | ||
cache.reset() | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idk what the use case for custom
reset
is, but we could add anonReset
callback instead of using a custom class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cache used by
splitVendorChunk
needs to be cleaned up on everybuildStart
. I don't understand theonReset
callback, how would that work?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the plugin would call
cache.clear
on the Map object and then callonReset
callback after that in case there's any custom logic needed. But as I said, I have zero idea why anyone would need custom reset logic here. Perhaps you have an idea? 😄There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh here's a use case. A plugin that wraps the
splitVendorChunk
plugin may need to be reset its own cache.This example needs
manualChunks
andonReset
options to be added tosplitVendorChunkPlugin
though. Not sure if this is what you had in mind for thereset
method though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user is already doing a new plugin, I think it is better that they use the function
splitVendorChunk
and they can reset their caches at the same time as resetting theSplitVendorChunkCache
(onbuildStart
). I don't think we need to have an API for the plugin. The plugin is there so users can add in a single line the old strategy, and we are exposing the low level function because it was requested several times before to compose it with other logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay that's fair, but I'm still confused why the
SplitVendorChunkCache
class is necessary if you can just callcache.clear
on the Map object.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right, you said:
That doesn't really make sense to me. If we decide not to use a Map in the future (not sure why we would tbh), then we can handle that then.
But if you really believe the cache class is the right move, I'll defer to your judgment, since this isn't a huge deal or anything. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just feels to me that we are leaking an implementation detail if we share the Map. In this case it isn't even something useful like a map of modules, it is an internal cache used by the algorithm. I also don't like having to create a class, but it feels safer here to avoid the need for a breaking change.