fix(resolve): check nested directories for package.json#5665
fix(resolve): check nested directories for package.json#5665patak-cat merged 2 commits intovitejs:mainfrom
Conversation
| const nestedPath = id.substring(lastArrowIndex + 1).trim() | ||
|
|
||
| // check for deep import, e.g. "my-lib/foo" | ||
| const deepMatch = nestedPath.match(deepImportRE) |
There was a problem hiding this comment.
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] === '@') { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Would you open an issue for it?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Also tryResolveFile does not handle deep imports, so the old implementation was incomplete
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So
pkgIdbecomesdevextreme/ui/shared
That would only be possible if devextreme/ui/shared/package.json existed, but it doesn't according to Unpkg.
There was a problem hiding this comment.
Oh sorry, then I'm mistaken on that one.
But the issue does get fixed after reverting this commit…
There was a problem hiding this comment.
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 {|
Hey chaps, I am so looking forward to see this issue get fixed. |
Description
This should fix the failing tests in #5662
Basically, when a deep import is encountered (eg:
vue/server-renderer), it should check forvue/server-renderer/package.jsonbefore checking forvue/package.json.What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).