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

Merged
merged 2 commits into from
Nov 13, 2021

Conversation

aleclarson
Copy link
Member

@aleclarson aleclarson commented Nov 12, 2021

Description

This should fix the failing tests in #5662

Basically, when a deep import is encountered (eg: vue/server-renderer), it should check for vue/server-renderer/package.json before checking for vue/package.json.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@aleclarson aleclarson added the p3-minor-bug An edge case that only affects very specific usage (priority) label Nov 12, 2021
@aleclarson aleclarson requested a review from patak-dev November 13, 2021 00:08
@@ -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

// 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

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.

Copy link
Member Author

@aleclarson aleclarson Jan 5, 2022

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

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

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/devextreme@21.2.4/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 {

@jzs11
Copy link

jzs11 commented Jan 14, 2022

Hey chaps, I am so looking forward to see this issue get fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-minor-bug An edge case that only affects very specific usage (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants