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(resolve): check nested directories for package.json #5665

Merged
merged 2 commits into from
Nov 13, 2021
Merged

fix(resolve): check nested directories for package.json #5665

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca80b40
fix(resolve): check nested directories for package.json
aleclarson Nov 12, 2021
a08d552
chore: fix typescript error
aleclarson Nov 13, 2021
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
65 changes: 49 additions & 16 deletions packages/vite/src/node/plugins/resolve.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import {
isBuiltin,
bareImportRE,
createDebugger,
deepImportRE,
injectQuery,
isExternalUrl,
isObject,
Expand Down Expand Up @@ -505,13 +504,35 @@ export function tryNodeResolve(
const nestedRoot = id.substring(0, lastArrowIndex).trim()
const nestedPath = id.substring(lastArrowIndex + 1).trim()

// check for deep import, e.g. "my-lib/foo"
const deepMatch = nestedPath.match(deepImportRE)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this no longer needs to be exported from utils. Not sure if it's worth changing that there or not

const possiblePkgIds: string[] = []
for (let prevSlashIndex = -1; ; ) {
let slashIndex = nestedPath.indexOf('/', prevSlashIndex + 1)
if (slashIndex < 0) {
slashIndex = nestedPath.length
}

const part = nestedPath.slice(
prevSlashIndex + 1,
(prevSlashIndex = slashIndex)
)
if (!part) {
break
}

const pkgId = deepMatch ? deepMatch[1] || deepMatch[2] : nestedPath
// Assume path parts with an extension are not package roots, except for the
// first path part (since periods are sadly allowed in package names).
// At the same time, skip the first path part if it begins with "@"
// (since "@foo/bar" should be treated as the top-level path).
if (possiblePkgIds.length ? path.extname(part) : part[0] === '@') {
Copy link
Member

@haoqunjiang haoqunjiang Dec 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sadly this causes bugs in the case of devextreme: it contains paths like devextreme/ui/shared/ui.editor_factory_mixin: https://unpkg.com/devextreme/ui/shared/ui.editor_factory_mixin/

Do you have a specific test case for the problem that this PR was trying to fix?
Because for imports such as vue/server-renderer, even if we try resolveDeepImport first, it will eventually process the nested package.json in the tryResolveFile function.
So it should work correctly even without this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Would you open an issue for it?

Copy link
Member

Choose a reason for hiding this comment

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

#6061

Copy link
Member Author

@aleclarson aleclarson Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So it should work correctly even without this PR.

Did you test that theory? If I remember right, the resolveExports call will throw when pkg.exports does not contain the (incorrectly assumed) deep import.

Copy link
Member Author

@aleclarson aleclarson Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also tryResolveFile does not handle deep imports, so the old implementation was incomplete

Copy link
Member

Choose a reason for hiding this comment

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

Have you manually confirmed #6061 is caused by this PR?

Yeah, I've manually confirmed.
I forgot whether it's this particular path that failed, but I'm sure it's in this function. The request goes into tryNodeResolve first and the calculated pkgId is wrong.

Copy link
Member

@haoqunjiang haoqunjiang Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

For require('devextreme/ui/shared/ui.editor_factory_mixin'),
possiblePkgIds becomes [ 'devextreme', 'devextreme/ui', 'devextreme/ui/shared' ]

Unfortunately, devextreme/ui/shared is a real directory path: https://unpkg.com/browse/[email protected]/ui/shared/

So pkgId becomes devextreme/ui/shared, which is wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

So pkgId becomes devextreme/ui/shared

That would only be possible if devextreme/ui/shared/package.json existed, but it doesn't according to Unpkg.

Copy link
Member

Choose a reason for hiding this comment

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

Oh sorry, then I'm mistaken on that one.

But the issue does get fixed after reverting this commit…

Copy link
Member

Choose a reason for hiding this comment

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

I don't think I have fully understood this issue.
But here's how I fixed #6061:

diff --git a/node_modules/vite/dist/node/chunks/dep-fcec4469.js b/node_modules/vite/dist/node/chunks/dep-fcec4469.js
index d16f7d4..828ef47 100644
--- a/node_modules/vite/dist/node/chunks/dep-fcec4469.js
+++ b/node_modules/vite/dist/node/chunks/dep-fcec4469.js
@@ -30385,28 +30385,12 @@ function tryNodeResolve(id, importer, options, targetWeb, server, ssr) {
     const lastArrowIndex = id.lastIndexOf('>');
     const nestedRoot = id.substring(0, lastArrowIndex).trim();
     const nestedPath = id.substring(lastArrowIndex + 1).trim();
-    const possiblePkgIds = [];
-    for (let prevSlashIndex = -1;;) {
-        let slashIndex = nestedPath.indexOf('/', prevSlashIndex + 1);
-        if (slashIndex < 0) {
-            slashIndex = nestedPath.length;
-        }
-        const part = nestedPath.slice(prevSlashIndex + 1, (prevSlashIndex = slashIndex));
-        if (!part) {
-            break;
-        }
-        // Assume path parts with an extension are not package roots, except for the
-        // first path part (since periods are sadly allowed in package names).
-        // At the same time, skip the first path part if it begins with "@"
-        // (since "@foo/bar" should be treated as the top-level path).
-        if (possiblePkgIds.length ? path__default.extname(part) : part[0] === '@') {
-            continue;
-        }
-        const possiblePkgId = nestedPath.slice(0, slashIndex);
-        possiblePkgIds.push(possiblePkgId);
-    }
+    // check for deep import, e.g. "my-lib/foo"
+    const deepImportRE = /^([^@][^/]*)\/|^(@[^/]+\/[^/]+)\//
+    const deepMatch = nestedPath.match(deepImportRE)
+    const pkgId = deepMatch ? deepMatch[1] || deepMatch[2] : nestedPath
     let basedir;
-    if (dedupe === null || dedupe === void 0 ? void 0 : dedupe.some((id) => possiblePkgIds.includes(id))) {
+    if (dedupe && dedupe.includes(pkgId)) {
         basedir = root;
     }
     else if (importer &&
@@ -30421,19 +30405,16 @@ function tryNodeResolve(id, importer, options, targetWeb, server, ssr) {
     if (nestedRoot) {
         basedir = nestedResolveFrom(nestedRoot, basedir, preserveSymlinks);
     }
-    let pkg;
-    const pkgId = possiblePkgIds.reverse().find((pkgId) => {
-        pkg = resolvePackageData(pkgId, basedir, preserveSymlinks, packageCache);
-        return pkg;
-    });
+    const pkg = resolvePackageData(pkgId, basedir, options.preserveSymlinks)
+
     if (!pkg) {
         return;
     }
     let resolveId = resolvePackageEntry;
-    let unresolvedId = pkgId;
-    if (unresolvedId !== nestedPath) {
+    let unresolvedId = id;
+    if (deepMatch) {
         resolveId = resolveDeepImport;
-        unresolvedId = '.' + nestedPath.slice(pkgId.length);
+        unresolvedId = '.' + id.slice(pkgId.length);
     }
     let resolved;
     try {

continue
}

const possiblePkgId = nestedPath.slice(0, slashIndex)
possiblePkgIds.push(possiblePkgId)
}

let basedir: string
if (dedupe && dedupe.includes(pkgId)) {
if (dedupe?.some((id) => possiblePkgIds.includes(id))) {
basedir = root
} else if (
importer &&
Expand All @@ -528,21 +549,33 @@ export function tryNodeResolve(
basedir = nestedResolveFrom(nestedRoot, basedir, options.preserveSymlinks)
}

const pkg = resolvePackageData(pkgId, basedir, options.preserveSymlinks)
let pkg: PackageData | undefined
const pkgId = possiblePkgIds.reverse().find((pkgId) => {
pkg = resolvePackageData(pkgId, basedir, options.preserveSymlinks)
return pkg
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
})!

if (!pkg) {
return
}

let resolved = deepMatch
? resolveDeepImport(
'.' + id.slice(pkgId.length),
pkg,
options,
targetWeb,
options.preserveSymlinks
)
: resolvePackageEntry(id, pkg, options, targetWeb, options.preserveSymlinks)
let resolved =
nestedPath !== pkgId
? resolveDeepImport(
'.' + nestedPath.slice(pkgId.length),
pkg,
options,
targetWeb,
options.preserveSymlinks
)
: resolvePackageEntry(
nestedPath,
pkg,
options,
targetWeb,
options.preserveSymlinks
)

if (!resolved) {
return
}
Expand Down Expand Up @@ -572,7 +605,7 @@ export function tryNodeResolve(
!isJsType ||
importer?.includes('node_modules') ||
exclude?.includes(pkgId) ||
exclude?.includes(id) ||
exclude?.includes(nestedPath) ||
SPECIAL_QUERY_RE.test(resolved) ||
ssr
) {
Expand Down