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(build): ensure linked packages are analyzed for commonjs #7094

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
57 changes: 41 additions & 16 deletions packages/vite/src/node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,7 @@ export type ResolvedConfig = Readonly<
env: Record<string, any>
resolve: ResolveOptions & {
alias: Alias[]
cjsInclude?: (string | RegExp)[]
}
plugins: readonly Plugin[]
server: ResolvedServerOptions
Expand Down Expand Up @@ -363,22 +364,6 @@ export async function resolveConfig(
{ find: /^[\/]?@vite\/client/, replacement: () => CLIENT_ENTRY }
]

// resolve alias with internal client alias
const resolvedAlias = normalizeAlias(
mergeAlias(
// @ts-ignore because @rollup/plugin-alias' type doesn't allow function
// replacement, but its implementation does work with function values.
clientAlias,
config.resolve?.alias || config.alias || []
)
)

const resolveOptions: ResolvedConfig['resolve'] = {
dedupe: config.dedupe,
...config.resolve,
alias: resolvedAlias
}

// load .env files
const envDir = config.envDir
? normalizePath(path.resolve(resolvedRoot, config.envDir))
Expand All @@ -400,6 +385,46 @@ export async function resolveConfig(
const BASE_URL = resolveBaseUrl(config.base, command === 'build', logger)
const resolvedBuildOptions = resolveBuildOptions(config.build)

let cjsInclude = resolvedBuildOptions.commonjsOptions.include
if (cjsInclude !== undefined) {
const include = (cjsInclude = arraify(cjsInclude))

// HACK: This uses an object that looks/acts like a RegExp
// but is actually dynamic so that symlinked files can be
// analyzed by CommonJS as their symlinks are resolved.
let cachedLength = include.length
let cachedFilter = createFilter(include)
resolvedBuildOptions.commonjsOptions.include = {
// Only this method is needed to be `createFilter` compatible.
// It assumes the `include` array length only ever increases if mutated.
test(id: string) {
if (include.length !== cachedLength) {
cachedLength = include.length
cachedFilter = createFilter(include)
}
return cachedFilter(id)
},
// Ensure "instanceof RegExp" returns true.
__proto__: RegExp.prototype
} as any
}

// resolve alias with internal client alias
const resolvedAlias = normalizeAlias(
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember why const resolvedAlias and const resolveOptions declarations were moved down. We could probably move them back.. :)

mergeAlias(
// @ts-ignore because @rollup/plugin-alias' type doesn't allow function
// replacement, but its implementation does work with function values.
clientAlias,
config.resolve?.alias || config.alias || []
)
)

const resolveOptions: ResolvedConfig['resolve'] = {
dedupe: config.dedupe,
...config.resolve,
alias: resolvedAlias
}

// resolve cache directory
const pkgPath = lookupFile(
resolvedRoot,
Expand Down
21 changes: 17 additions & 4 deletions packages/vite/src/node/packages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export function resolvePackageData(
id: string,
basedir: string,
preserveSymlinks = false,
packageCache?: PackageCache
packageCache?: PackageCache,
cjsInclude?: (string | RegExp)[]
): PackageData | null {
let pkg: PackageData | undefined
let cacheKey: string | undefined
Expand All @@ -60,8 +61,8 @@ export function resolvePackageData(
}
let pkgPath: string | undefined
try {
pkgPath = resolveFrom(`${id}/package.json`, basedir, preserveSymlinks)
pkg = loadPackageData(pkgPath, true, packageCache)
pkgPath = resolveFrom(`${id}/package.json`, basedir, true)
Copy link
Member Author

@aleclarson aleclarson Feb 25, 2022

Choose a reason for hiding this comment

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

ℹ︎ Preserve symlinks when using resolveFrom so we have the symlink path inside loadPackageData.

pkg = loadPackageData(pkgPath, preserveSymlinks, packageCache, cjsInclude)
if (packageCache) {
packageCache.set(cacheKey!, pkg)
}
Expand All @@ -81,10 +82,22 @@ export function resolvePackageData(
export function loadPackageData(
pkgPath: string,
preserveSymlinks?: boolean,
packageCache?: PackageCache
packageCache?: PackageCache,
cjsInclude?: (string | RegExp)[]
): PackageData {
if (!preserveSymlinks) {
const originalPkgPath = pkgPath
pkgPath = fs.realpathSync.native(pkgPath)

// In case a linked package is a local clone of a CommonJS dependency,
// we need to ensure @rollup/plugin-commonjs analyzes the package even
// after it's been resolved to its actual file location.
if (cjsInclude && pkgPath !== originalPkgPath) {
const filter = createFilter(cjsInclude, undefined, { resolve: false })
if (!filter(pkgPath) && filter(originalPkgPath)) {
cjsInclude.push(path.dirname(pkgPath) + '/**')
Copy link
Member Author

Choose a reason for hiding this comment

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

Possible downside here is that node_modules might be inadvertently matched? It's not easy to determine the source folder in a package, so might have to live with it.

}
}
}

let cached: PackageData | undefined
Expand Down
13 changes: 12 additions & 1 deletion packages/vite/src/node/plugins/resolve.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ export interface InternalResolveOptions extends ResolveOptions {
isProduction: boolean
ssrConfig?: SSROptions
packageCache?: PackageCache
cjsInclude?: (string | RegExp)[]
/**
* src code mode also attempts the following:
* - resolving /xxx as URLs
Expand All @@ -81,6 +82,7 @@ export interface InternalResolveOptions extends ResolveOptions {
export function resolvePlugin(baseOptions: InternalResolveOptions): Plugin {
const {
root,
isBuild,
isProduction,
asSrc,
ssrConfig,
Expand Down Expand Up @@ -120,6 +122,9 @@ export function resolvePlugin(baseOptions: InternalResolveOptions): Plugin {
...baseOptions,
isFromTsImporter: isTsRequest(importer ?? '')
}
if (!options.isBuild) {
options.cjsInclude = undefined
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember why cjsInclude is disabled in dev mode, but it would be nice to figure out why and add a comment here.. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose it's because @rollup/plugin-commonjs isn't used in dev mode 😆

}

let res: string | PartialResolvedId | undefined

Expand Down Expand Up @@ -520,7 +525,13 @@ export function tryNodeResolve(

let pkg: PackageData | undefined
const pkgId = possiblePkgIds.reverse().find((pkgId) => {
pkg = resolvePackageData(pkgId, basedir, preserveSymlinks, packageCache)!
pkg = resolvePackageData(
pkgId,
basedir,
preserveSymlinks,
packageCache,
options.cjsInclude
)!
return pkg
})!

Expand Down
2 changes: 1 addition & 1 deletion packages/vite/src/node/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,7 @@ export function resolveHostname(
return { host, name }
}

export function arraify<T>(target: T | T[]): T[] {
export function arraify<T>(target: T | readonly T[]): T[] {
return Array.isArray(target) ? target : [target]
}

Expand Down