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(ssr): no external symlink package #9296

Merged
merged 4 commits into from Jul 22, 2022
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 1 addition & 1 deletion packages/vite/src/node/plugins/resolve.ts
Expand Up @@ -87,7 +87,7 @@ export interface InternalResolveOptions extends ResolveOptions {
ssrOptimizeCheck?: boolean
// Resolve using esbuild deps optimization
getDepsOptimizer?: (ssr: boolean) => DepsOptimizer | undefined
shouldExternalize?: (id: string) => boolean | undefined
shouldExternalize?: (id: string) => boolean
}

export function resolvePlugin(resolveOptions: InternalResolveOptions): Plugin {
Expand Down
54 changes: 32 additions & 22 deletions packages/vite/src/node/ssr/ssrExternal.ts
Expand Up @@ -90,13 +90,13 @@ const _require = createRequire(import.meta.url)

const isSsrExternalCache = new WeakMap<
ResolvedConfig,
(id: string) => boolean | undefined
(id: string) => boolean
>()

export function shouldExternalizeForSSR(
id: string,
config: ResolvedConfig
): boolean | undefined {
): boolean {
let isSsrExternal = isSsrExternalCache.get(config)
if (!isSsrExternal) {
isSsrExternal = createIsSsrExternal(config)
Expand Down Expand Up @@ -151,46 +151,56 @@ export function createIsConfiguredAsSsrExternal(
}
}

function createIsSsrExternal(
config: ResolvedConfig
): (id: string) => boolean | undefined {
const processedIds = new Map<string, boolean | undefined>()

const { ssr, root } = config
function createIsSsrExternal(config: ResolvedConfig): (id: string) => boolean {
const processedIds = new Map<string, boolean>()

const isConfiguredAsExternal = createIsConfiguredAsSsrExternal(config)

const resolveOptions: InternalResolveOptions = {
root,
root: config.root,
preserveSymlinks: config.resolve.preserveSymlinks,
isProduction: false,
isBuild: true
}

const isExternalizable = (id: string) => {
if (!bareImportRE.test(id) || id.includes('\0')) {
return false
}
return !!tryNodeResolve(
const resolve = (id: string) => {
return tryNodeResolve(
id,
undefined,
resolveOptions,
ssr?.target === 'webworker',
config.ssr?.target === 'webworker',
undefined,
true,
true // try to externalize, will return undefined if not possible
false
Copy link
Member

Choose a reason for hiding this comment

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

We need to pass true here or the branch testing deep imports will not be evaluated

Copy link
Member

Choose a reason for hiding this comment

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

It is also filtering .css files when passing true, we need more tests in ssr-deps 🤔

Copy link
Member Author

@bluwy bluwy Jul 22, 2022

Choose a reason for hiding this comment

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

Hmm I changed to false so we can resolve to the full path to analyze node_modules though. I'll try to find another way for this. Otherwise resolving linked-dep would return linked-dep, which doesn't give enough heuristic

Copy link
Member

Choose a reason for hiding this comment

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

You could do these checks inside tryNodeResolve when passing true

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the hint. Now the fix is a lot simpler than before 😅

Copy link
Member

Choose a reason for hiding this comment

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

Nice! I think we still need a refactoring to make the branches more clear, we can get there in 3.1

)
}

const isExternal = (id: string) => {
if (id.startsWith('.') || path.isAbsolute(id) || isBuiltin(id)) {
return false
}
// check config for external/noExternal, undefined if not explicitly configured
const isConfigExternal = isConfiguredAsExternal(id)
if (isConfigExternal !== undefined) {
return isConfigExternal
}
// no external non-dep import or virtual files
if (!bareImportRE.test(id) || id.includes('\0')) {
return false
}
// no external if can't resolve or is symlink package
const resolved = resolve(id)
if (!resolved || !resolved.id.includes('node_modules')) {
return false
}
return true
}

return (id: string) => {
if (processedIds.has(id)) {
return processedIds.get(id)
}
let external = false
if (!id.startsWith('.') && !path.isAbsolute(id)) {
external =
isBuiltin(id) || (isConfiguredAsExternal(id) ?? isExternalizable(id))
return processedIds.get(id)!
}
const external = isExternal(id)
processedIds.set(id, external)
return external
}
Expand Down
5 changes: 5 additions & 0 deletions playground/ssr-deps/__tests__/ssr-deps.spec.ts
Expand Up @@ -98,3 +98,8 @@ test('msg from external using external entry', async () => {
'Hello World!'
)
})

test('msg from linked no external', async () => {
await page.goto(url)
expect(await page.textContent('.linked-no-external')).toMatch('Hello World!')
})
5 changes: 5 additions & 0 deletions playground/ssr-deps/linked-no-external/index.js
@@ -0,0 +1,5 @@
export const hello = function () {
// make sure linked package is not externalized so Vite features like
// import.meta.env works (or handling TS files)
return import.meta.env.DEV && 'Hello World!'
}
6 changes: 6 additions & 0 deletions playground/ssr-deps/linked-no-external/package.json
@@ -0,0 +1,6 @@
{
"name": "linked-no-external",
"private": true,
"type": "module",
"version": "0.0.0"
}
3 changes: 2 additions & 1 deletion playground/ssr-deps/package.json
Expand Up @@ -26,7 +26,8 @@
"optimized-with-nested-external": "file:./optimized-with-nested-external",
"optimized-cjs-with-nested-external": "file:./optimized-with-nested-external",
"external-using-external-entry": "file:./external-using-external-entry",
"external-entry": "file:./external-entry"
"external-entry": "file:./external-entry",
"linked-no-external": "link:./linked-no-external"
},
"devDependencies": {
"cross-env": "^7.0.3",
Expand Down
4 changes: 4 additions & 0 deletions playground/ssr-deps/src/app.js
Expand Up @@ -11,6 +11,7 @@ import onlyObjectAssignedExports from 'only-object-assigned-exports'
import requireAbsolute from 'require-absolute'
import noExternalCjs from 'no-external-cjs'
import importBuiltinCjs from 'import-builtin-cjs'
import { hello as linkedNoExternal } from 'linked-no-external'

// This import will set a 'Hello World!" message in the nested-external non-entry dependency
import 'non-optimized-with-nested-external'
Expand Down Expand Up @@ -75,5 +76,8 @@ export async function render(url, rootDir) {
const externalUsingExternalEntryMessage = externalUsingExternalEntry.hello()
html += `\n<p class="external-using-external-entry">message from external-using-external-entry: ${externalUsingExternalEntryMessage}</p>`

const linkedNoExternalMessage = linkedNoExternal()
html += `\n<p class="linked-no-external">message from linked-no-external: ${linkedNoExternalMessage}</p>`

return html + '\n'
}
5 changes: 5 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.