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

Merged
merged 1 commit into from Jul 14, 2021

Conversation

@Yelmor
Copy link
Contributor

@Yelmor Yelmor commented Jul 3, 2021

Description

Fix #3254.

Correct fixes in #3753 which caused #4005, #4014.

It's still a "easy fix". I think it's not necessary to use another esbuildScanPlugin to fix this. For non-entry import, it's ok to just leave it to "vite's own resolver".

I added a test case to fit the circumstance in #4005, and I also checked the reproduction of #4005 and #4014, they worked fine in the new fix.

Additional context

As I didn't read through the whole code base of vite, there may still be unconsidered circumstances causing error.


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.
@@ -85,35 +85,29 @@ export function esbuildDepPlugin(
}
)

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

This comment has been minimized.

@patak-js

patak-js Jul 4, 2021
Member

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

This comment has been minimized.

@Yelmor

Yelmor Jul 4, 2021
Author Contributor

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

This comment has been minimized.

@Yelmor

Yelmor Jul 4, 2021
Author Contributor

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.

This comment has been minimized.

@director-zhou

director-zhou Jul 4, 2021

Indeed, this code is meaningless. Is there a good fix now

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

This comment has been minimized.

@Yelmor

Yelmor Jul 4, 2021
Author Contributor

Too many issues.

I mean #3053 on third version.

This comment has been minimized.

@patak-js

patak-js Jul 4, 2021
Member

Thanks for taking the time to write down the detailed history of the issue. This is very helpful.

@eudinnou
Copy link

@eudinnou eudinnou commented Jul 14, 2021

Hei, we are doing some tests to migrate from snowpack to vite and this issue is a blocker for us. Is there a timeline for when this PR will get merged? Thanks

@patak-js patak-js changed the title Resolve nested dependencies from sub node_modules (fix #3254) fix: nested dependencies from sub node_modules, fix #3254 Jul 14, 2021
@patak-js patak-js merged commit b465d3e into vitejs:main Jul 14, 2021
6 checks passed
6 checks passed
@github-actions
Build&Test: node-12, ubuntu-latest
Details
@github-actions
Build&Test: node-14, ubuntu-latest
Details
@github-actions
Build&Test: node-16, ubuntu-latest
Details
@github-actions
Build&Test: node-14, macos-latest
Details
@github-actions
Build&Test: node-14, windows-latest
Details
@github-actions
Lint: node-14, ubuntu-latest
Details
@patak-js patak-js mentioned this pull request Jul 21, 2021
6 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants