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: nested dependencies from sub node_modules, fix #3254 #4091
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
6 changes: 4 additions & 2 deletions
6
packages/playground/nested-deps/__tests__/nested-deps.spec.ts
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,8 @@ | ||
// TODO: Rework #3753, taking into account issues with #4005, #4012, #4014 | ||
test.skip('handle nested package', async () => { | ||
test('handle nested package', async () => { | ||
expect(await page.textContent('.a')).toBe('A@2.0.0') | ||
expect(await page.textContent('.b')).toBe('B@1.0.0') | ||
expect(await page.textContent('.nested-a')).toBe('A@1.0.0') | ||
const c = await page.textContent('.c') | ||
expect(c).toBe('es-C@1.0.0') | ||
expect(await page.textContent('.side-c')).toBe(c) | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export default 'es-C@1.0.0' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
// this module should not be resolved | ||
export default 'C@1.0.0' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
{ | ||
"name": "test-package-c", | ||
"version": "1.0.0", | ||
"main": "index.js", | ||
"module": "index-es.js" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { default as C } from 'test-package-c' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolveDir
was added here to solve #2975, in this PR #3003 (comment)Would you check that this is no longer an issue after removing it? It would be great to add a test based on #2975 to your growing deps test suite (thanks for that by the way!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolveDir
is removed on purpose.I can describe the history of this bug.
#2975 and #3254 targets to same bug introduced in 8e1d3d8.
The original purpose of 8e1d3d8 is to aviod duplicated modules by separate entry imports from none-entry imports.
It introduced first version of
resolveEntry
util function:For none-entry import, using
qualified[flatId]
would make nested dependency likenode_modules/package-a/node_modules/package-b
never be resolved.Then #3003 comes to fix. It uses nodejs api
require.resolve
and resolveDir (provided by esbuild) to resolve none-entry import from sub node_modules directory.resolveDir
introduced in #3003:#3003 has two problems. The more obvious one is
flatId
+require.resolve
= module not found. For scoped packages like@some/package
,flatId
would be likesome_package
. And nodejs'srequire.resolve
can't resolve the flatted id.So we have third version of
resolveEntry
in #3053:#3053 shouldn't be merged, because it breaks fix of #3003. It resolved scoped package problem, but
qualified[flatId]
is a absolute path point tonode_modules/xx/main-of-xx
. Therequire.resolve
andresolveDir
are pointless.Then I comes in #3753:
It looks good, but in
2.4.0-beta.0
beta release we got error reports from #4005 and #4014. The problem isrequire.resolve
behaves different with vite's module resolve function. For packages provide es module ("module" in package.json), it still resolve to "main". It's actually the other one problem introduced in #3003.And as you see this(#4091) is the most recent fix on the long living bug. As described, I think it's not necessary to process none-entry imports in
resolveEntry
, so resolveDir is removed.What a long story= =
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the first version of test case(
test-package-a
andtest-package-b
) is very suit to the circumstance of #2975, so I don't think we needs another test case for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, this code is meaningless. Is there a good fix now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Too many issues.
I mean #3053 on third version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking the time to write down the detailed history of the issue. This is very helpful.