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: nested dependencies from sub node_modules, fix #3254 #4091

Merged
merged 1 commit into from Jul 14, 2021
Merged

fix: nested dependencies from sub node_modules, fix #3254 #4091

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/playground/nested-deps/__tests__/nested-deps.spec.ts
View file
Open in desktop
@@ -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)
})
11 changes: 11 additions & 0 deletions packages/playground/nested-deps/index.html
View file
Open in desktop
Expand Up @@ -7,14 +7,25 @@ <h2>direct dependency B</h2>
<h2>nested dependency A</h2>
<pre class="nested-a"></pre>

<h2>direct dependency C</h2>
<pre class="c"></pre>

<h2>side dependency C</h2>
<pre class="side-c"></pre>

<script type="module">
import A from 'test-package-a'
import B, { A as nestedA } from 'test-package-b'
import C from 'test-package-c'
import { C as sideC } from 'test-package-c/side'

text('.a', A)
text('.b', B)
text('.nested-a', nestedA)

text('.c', C)
text('.side-c', sideC)

function text(sel, text) {
document.querySelector(sel).textContent = text
}
Expand Down
3 changes: 2 additions & 1 deletion packages/playground/nested-deps/package.json
View file
Open in desktop
Expand Up @@ -10,6 +10,7 @@
},
"dependencies": {
"test-package-a": "link:./test-package-a",
"test-package-b": "link:./test-package-b"
"test-package-b": "link:./test-package-b",
"test-package-c": "link:./test-package-c"
}
}
1 change: 1 addition & 0 deletions packages/playground/nested-deps/test-package-c/index-es.js
View file
Open in desktop
@@ -0,0 +1 @@
export default 'es-C@1.0.0'
2 changes: 2 additions & 0 deletions packages/playground/nested-deps/test-package-c/index.js
View file
Open in desktop
@@ -0,0 +1,2 @@
// this module should not be resolved
export default 'C@1.0.0'
6 changes: 6 additions & 0 deletions packages/playground/nested-deps/test-package-c/package.json
View file
Open in desktop
@@ -0,0 +1,6 @@
{
"name": "test-package-c",
"version": "1.0.0",
"main": "index.js",
"module": "index-es.js"
}
1 change: 1 addition & 0 deletions packages/playground/nested-deps/test-package-c/side.js
View file
Open in desktop
@@ -0,0 +1 @@
export { default as C } from 'test-package-c'
7 changes: 6 additions & 1 deletion packages/playground/nested-deps/vite.config.js
View file
Open in desktop
Expand Up @@ -3,6 +3,11 @@
*/
module.exports = {
optimizeDeps: {
include: ['test-package-a', 'test-package-b']
include: [
'test-package-a',
'test-package-b',
'test-package-c',
'test-package-c/side'
]
}
}
32 changes: 13 additions & 19 deletions packages/vite/src/node/optimizer/esbuildDepPlugin.ts
View file
Open in desktop
Expand Up @@ -85,35 +85,29 @@ export function esbuildDepPlugin(
}
)

function resolveEntry(id: string, isEntry: boolean, resolveDir: string) {
Copy link
Member

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!)

Copy link
Contributor Author

@Yelmor Yelmor Jul 4, 2021 •
edited

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:

function resolveEntry(id: string, isEntry: boolean) {
        const flatId = flattenId(id)
        if (flatId in qualified) {
          return isEntry
            ? {
                path: flatId,
                namespace: 'dep'
              }
            : {
                path: path.resolve(qualified[flatId])
              }
        }
      }

For none-entry import, using qualified[flatId] would make nested dependency like node_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:

      function resolveEntry(id: string, isEntry: boolean, resolveDir: string) {
        const flatId = flattenId(id)
        if (flatId in qualified) {
          return isEntry
            ? {
                path: flatId,
                namespace: 'dep'
              }
            : {
                path: require.resolve(flatId, {
                  paths: [resolveDir]
                })
              }
        }
      }

#3003 has two problems. The more obvious one is flatId + require.resolve = module not found. For scoped packages like @some/package, flatId would be like some_package. And nodejs's require.resolve can't resolve the flatted id.

So we have third version of resolveEntry in #3053:

      function resolveEntry(id: string, isEntry: boolean, resolveDir: string) {
        const flatId = flattenId(id)
        if (flatId in qualified) {
          return isEntry
            ? {
                path: flatId,
                namespace: 'dep'
              }
            : {
                path: require.resolve(qualified[flatId], { // flatId -> qualified[flatId]
                  paths: [resolveDir]
                })
              }
        }
      }

#3053 shouldn't be merged, because it breaks fix of #3003. It resolved scoped package problem, but qualified[flatId] is a absolute path point to node_modules/xx/main-of-xx. The require.resolve and resolveDir are pointless.

Then I comes in #3753:

      function resolveEntry(id: string, isEntry: boolean, resolveDir: string) {
        const flatId = flattenId(id)
        if (flatId in qualified) {
          return isEntry
            ? {
                path: flatId,
                namespace: 'dep'
              }
            : {
                path: require.resolve(id, { // qualified[flatId] -> id
                  paths: [resolveDir]
                })
              }
        }
      }

It looks good, but in 2.4.0-beta.0 beta release we got error reports from #4005 and #4014. The problem is require.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= =

Copy link
Contributor Author

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 and test-package-b) is very suit to the circumstance of #2975, so I don't think we needs another test case for this.

Copy link

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

{
    path: require.resolve(id, { // qualified[flatId] -> id
      paths: [resolveDir]
    })
  }

Copy link
Contributor Author

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.

Copy link
Member

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.

function resolveEntry(id: string) {
const flatId = flattenId(id)
if (flatId in qualified) {
return isEntry
? {
path: flatId,
namespace: 'dep'
}
: {
path: require.resolve(qualified[flatId], {
paths: [resolveDir]
})
}
return {
path: flatId,
namespace: 'dep'
}
}
}

build.onResolve(
{ filter: /^[\w@][^:]/ },
async ({ path: id, importer, kind, resolveDir }) => {
const isEntry = !importer
async ({ path: id, importer, kind }) => {
// ensure esbuild uses our resolved entries
let entry
// if this is an entry, return entry namespace resolve result
if ((entry = resolveEntry(id, isEntry, resolveDir))) return entry

// check if this is aliased to an entry - also return entry namespace
const aliased = await _resolve(id, undefined, true)
if (aliased && (entry = resolveEntry(aliased, isEntry, resolveDir))) {
return entry
if (!importer) {
if ((entry = resolveEntry(id))) return entry
// check if this is aliased to an entry - also return entry namespace
const aliased = await _resolve(id, undefined, true)
if (aliased && (entry = resolveEntry(aliased))) {
return entry
}
}

// use vite's own resolver
Expand Down
4 changes: 4 additions & 0 deletions yarn.lock
View file
Open in desktop
Expand Up @@ -6962,6 +6962,10 @@ test-exclude@^6.0.0:
version "0.0.0"
uid ""

"test-package-c@link:./packages/playground/nested-deps/test-package-c":
version "0.0.0"
uid ""

text-extensions@^1.0.0:
version "1.9.0"
resolved "https://registry.yarnpkg.com/text-extensions/-/text-extensions-1.9.0.tgz#1853e45fee39c945ce6f6c36b2d659b5aabc2a26"
Expand Down