fix(build): ensure linked packages are analyzed for commonjs #7094
Conversation
…if the symlink pointing to them is matched by any of the `commonjsOptions.include` patterns.
| } | ||
|
|
||
| // resolve alias with internal client alias | ||
| const resolvedAlias = normalizeAlias( |
There was a problem hiding this comment.
I don't remember why const resolvedAlias and const resolveOptions declarations were moved down. We could probably move them back.. :)
| @@ -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) | |||
There was a problem hiding this comment.
ℹ︎ Preserve symlinks when using resolveFrom so we have the symlink path inside loadPackageData.
| if (cjsInclude && pkgPath !== originalPkgPath) { | ||
| const filter = createFilter(cjsInclude, undefined, { resolve: false }) | ||
| if (!filter(pkgPath) && filter(originalPkgPath)) { | ||
| cjsInclude.push(path.dirname(pkgPath) + '/**') |
There was a problem hiding this comment.
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.
| @@ -120,6 +122,9 @@ export function resolvePlugin(baseOptions: InternalResolveOptions): Plugin { | |||
| ...baseOptions, | |||
| isFromTsImporter: isTsRequest(importer ?? '') | |||
| } | |||
| if (!options.isBuild) { | |||
| options.cjsInclude = undefined | |||
There was a problem hiding this comment.
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.. :)
There was a problem hiding this comment.
I suppose it's because @rollup/plugin-commonjs isn't used in dev mode 😆
|
Is there any chance that this fix will pass in v2.9 ? It’s needed to migrate my CRA to vite. |
|
@TeckTn until this is relased, if it will be check out this section https://vitejs.dev/guide/dep-pre-bundling.html#monorepos-and-linked-dependencies |
|
@aleclarson out of curiosity, will this fix #2139 once merged? :) |
|
@fwouts No I don't think it will. IIRC, the issue there is related to |
|
What can we do to help get this PR merged? :) |
|
IIUC this only automates setting I'm not sure if we want to merge a partial solution, and the regex trick is a bit hacky. Maybe it would be better to retrieve linked packages while performing a scan, and have the Vite dev server and build handle accordingly. |
|
@fwouts is this still an issue when using esbuild deps optimization at build time? I think we may want to avoid adding more complexity to the rollup plugin commonjs if it wont be recommended in the future |
I haven't tried it yet (sorry haven't had the bandwidth) but I expect it wouldn't be anymore! |
|
We are facing the same issue and applying the proposed solution in docs is not enough. It does solve it for build time, but not dev time. Any progress on this? |
|
@datrinh have you tried Vite 3? |
|
Even after referring the docs for this our issue was not resolved, We are using Vite 3.0.4. Any update on this will be helpful. |
|
same as @shubhamsWEB |
|
Closing as this is outdated, with some concerns noted, and the linked issues are already closed with docs for now. |
Description
Ensure linked packages are analyzed for commonjs if the symlink pointing to them is matched by any of the
commonjsOptions.includepatterns.Additional context
Fixes #5668
Fixes #2697
Thanks to @fwouts for sponsoring this PR :)
What is the purpose of this pull request?