Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: allow inlining vite's cached dependencies #6284

Merged
merged 4 commits into from
Sep 4, 2024
Merged

fix: allow inlining vite's cached dependencies #6284

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4648724
Allow inlining pre-bundled deps
chriswheeldon-peakon Jul 19, 2024
90bf322
Update comment
chriswheeldon-peakon Aug 5, 2024
3d0adbc
Another refinement of comment
chriswheeldon-peakon Aug 5, 2024
8c78068
Add test cases for shouldExternalize
chriswheeldon-peakon Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions packages/vite-node/src/externalize.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,6 @@ async function _shouldExternalize(

id = patchWindowsImportPath(id)

// always externalize Vite deps, they are too big to inline
if (options?.cacheDir && id.includes(options.cacheDir)) {
return id
}

const moduleDirectories = options?.moduleDirectories || ['/node_modules/']

if (matchExternalizePattern(id, moduleDirectories, options?.inline)) {
Expand All @@ -125,6 +120,12 @@ async function _shouldExternalize(
return id
}

// Unless the user explicitly opted to inline them, externalize Vite deps.
// They are too big to inline by default.
if (options?.cacheDir && id.includes(options.cacheDir)) {
return id
}

const isLibraryModule = moduleDirectories.some(dir => id.includes(dir))
const guessCJS = isLibraryModule && options?.fallbackCJS
id = guessCJS ? guessCJSversion(id) || id : id
Expand Down
52 changes: 52 additions & 0 deletions test/vite-node/test/server.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,3 +229,55 @@ describe('server correctly caches data', () => {
expect(webFiles).toHaveLength(1)
})
})

describe('externalize', () => {
describe('by default', () => {
test('should externalize vite\'s cached dependencies', async () => {
const vnServer = new ViteNodeServer({
config: {
root: '/',
cacheDir: '/node_modules/.vite',
},
} as any, {})

const externalize = await vnServer.shouldExternalize('/node_modules/.vite/cached.js')
expect(externalize).toBeTruthy()
})
})

describe('with server.deps.inline: true', () => {
test('should not externalize vite\'s cached dependencies', async () => {
Comment on lines +248 to +249
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sheremet-va, what do you think about inline: true also inlining the cached dependencies? I worry that this makes this change potentially much more impactful for existing users of vitest who use inline: true. I could perhaps make it so when evaluating if the id matches the cache dir we only consider paths and patterns from deps.inline i.e. require the user to be explicit about inlining the cache deps.

Copy link
Member

@sheremet-va sheremet-va Aug 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only impact they'd have is slower tests. inline: true is already very slow by design, so it should be fine to inline deps with inline: true

const vnServer = new ViteNodeServer({
config: {
root: '/',
cacheDir: '/node_modules/.vite',
},
} as any, {
deps: {
inline: true,
},
})

const externalize = await vnServer.shouldExternalize('/node_modules/.vite/cached.js')
expect(externalize).toBeFalsy()
})
})

describe('with server.deps.inline including the cache dir', () => {
test('should not externalize vite\'s cached dependencies', async () => {
const vnServer = new ViteNodeServer({
config: {
root: '/',
cacheDir: '/node_modules/.vite',
},
} as any, {
deps: {
inline: [/node_modules\/\.vite/],
},
})

const externalize = await vnServer.shouldExternalize('/node_modules/.vite/cached.js')
expect(externalize).toBeFalsy()
})
})
})
Loading