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.

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

fix(browser): don't exclude node builtins from optimization #5082

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 6 additions & 15 deletions packages/browser/src/node/esmInjector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ export function injectVitestModule(code: string, id: string, parse: PluginContex

const hoistIndex = 0

let hasInjected = false

// this will transform import statements into dynamic ones, if there are imports
// it will keep the import as is, if we don't need to mock anything
// in browser environment it will wrap the module value with "vitest_wrap_module" function
Expand Down Expand Up @@ -76,7 +74,6 @@ export function injectVitestModule(code: string, id: string, parse: PluginContex
}

function defineExport(position: number, name: string, local = name) {
hasInjected = true
s.appendLeft(
position,
`\nObject.defineProperty(${viInjectedKey}, "${name}", `
Expand Down Expand Up @@ -170,7 +167,6 @@ export function injectVitestModule(code: string, id: string, parse: PluginContex
// named hoistable/class exports
// export default function foo() {}
// export default class A {}
hasInjected = true
const { name } = node.declaration.id
s.remove(node.start, node.start + 15 /* 'export default '.length */)
s.append(
Expand All @@ -180,7 +176,6 @@ export function injectVitestModule(code: string, id: string, parse: PluginContex
}
else {
// anonymous default exports
hasInjected = true
s.update(
node.start,
node.start + 14 /* 'export default'.length */,
Expand All @@ -196,13 +191,10 @@ export function injectVitestModule(code: string, id: string, parse: PluginContex
s.remove(node.start, node.end)
const importId = defineImportAll(node.source.value as string)
// hoist re-exports near the defined import so they are immediately exported
if (node.exported) {
if (node.exported)
defineExport(hoistIndex, node.exported.name, `${importId}`)
}
else {
hasInjected = true
else
s.appendLeft(hoistIndex, `${viExportAllHelper}(${viInjectedKey}, ${importId});\n`)
}
}
}

Expand Down Expand Up @@ -244,11 +236,10 @@ export function injectVitestModule(code: string, id: string, parse: PluginContex
},
})

if (hasInjected) {
// make sure "__vi_injected__" is declared as soon as possible
s.prepend(`const ${viInjectedKey} = { [Symbol.toStringTag]: "Module" };\n`)
s.append(`\nexport { ${viInjectedKey} }`)
}
// make sure "__vi_injected__" is declared as soon as possible
// prepend even if file doesn't export anything
s.prepend(`const ${viInjectedKey} = { [Symbol.toStringTag]: "Module" };\n`)
s.append(`\nexport { ${viInjectedKey} }`)

return {
ast,
Expand Down
13 changes: 10 additions & 3 deletions packages/browser/src/node/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { fileURLToPath } from 'node:url'

import { resolve } from 'node:path'
import { builtinModules } from 'node:module'
import sirv from 'sirv'
import type { Plugin } from 'vite'
import type { WorkspaceProject } from 'vitest/node'
Expand Down Expand Up @@ -61,18 +60,19 @@ export default (project: WorkspaceProject, base = '/'): Plugin[] => {
'vitest/runners',
],
exclude: [
...builtinModules,
'vitest',
'vitest/utils',
'vitest/browser',
'vitest/runners',
'@vitest/utils',

// loupe is manually transformed
'loupe',
],
include: [
'vitest > @vitest/utils > pretty-format',
'vitest > @vitest/snapshot > pretty-format',
'vitest > diff-sequences',
'vitest > loupe',
'vitest > pretty-format',
'vitest > pretty-format > ansi-styles',
'vitest > pretty-format > ansi-regex',
Expand All @@ -81,6 +81,13 @@ export default (project: WorkspaceProject, base = '/'): Plugin[] => {
},
}
},
transform(code, id) {
if (id.includes('loupe/loupe.js')) {
Copy link
Member Author

Choose a reason for hiding this comment

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

We can just bundle loupe to be honest, but then we should probably bundle chai, but I don't see that happening 🤷🏻

Copy link
Contributor

Choose a reason for hiding this comment

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

Just in case, can you clarify the issues with bundling chai?
I guess the obvious one is that it affects vitest's package size. Another one I thought about is that a user code relying on import "chai" would end up with different instance of chai from what Vitest sees internally.
Do you have anything else in mind?

const exportsList = ['custom', 'inspect', 'registerConstructor', 'registerStringTag']
const codeAppend = exportsList.map(i => `export const ${i} = globalThis.loupe.${i}`).join('\n')
return `${code}\n${codeAppend}\nexport default globalThis.loupe`
}
},
async resolveId(id) {
if (!/\?browserv=\w+$/.test(id))
return
Expand Down
4 changes: 2 additions & 2 deletions packages/utils/src/display.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// since this is already part of Vitest via Chai, we can just reuse it without increasing the size of bundle
// @ts-expect-error doesn't have types
import { inspect as loupe } from 'loupe'
import * as loupe from 'loupe'

interface LoupeOptions {
showHidden?: boolean | undefined
Expand Down Expand Up @@ -101,7 +101,7 @@ export function format(...args: unknown[]) {
export function inspect(obj: unknown, options: LoupeOptions = {}): string {
if (options.truncate === 0)
options.truncate = Number.POSITIVE_INFINITY
return loupe(obj, options)
return loupe.inspect(obj, options)
}

export function objDisplay(obj: unknown, options: LoupeOptions = {}): string {
Expand Down