Bad heuristic used to differentiate between in-repo and dependency code

[wburgin-anduril]

Describe the bug

Vite uses isInNodeModules() as a heuristic to differentiate between in-repo code and dependency code during dependency pre-bundling:

export function isInNodeModules(id: string): boolean {
  return id.includes('node_modules')
}

PNPM's dependenciesMeta.*.injected feature violates this heuristic by resolving both in-repo and NPM dependencies to paths that include node_modules. When this feautre is used, @monorepo/foo-lib from the linked repro resolves to node_modules/.pnpm/file+pkgs+foo-lib/node_modules/@monorepo/foo-lib/dist/index.js, which causes isInNodeModules() to be true and triggers failures.

The sandbox environment setup by https://github.com/aspect-build/rules_js to run Vite with Bazel behaves very similarly. For example in the linked repro, @monorepo/foo-lib ultimately resolves to bazel-bin/node_modules/.aspect_rules_js/@[email protected]/node_modules/@monorepo/foo-lib/dist/index.js, which means isInNodeModules() will be true.

I originally assumed that this would affect Plug 'n Play environments as well since they resolve dependencies straight out of .zip files, but it looks like we're getting lucky that the ids in that case still include node_modules. For example: ~/wburgin-anduril/vite-repro/.yarn/cache/chalk-npm-5.3.0-d181999efb-8297d436b2.zip/node_modules/chalk/source/index.js. Maybe that's as a result of issues with heuristics like this 🤔 🤷‍♂️.

---

We haven't had any luck using optimizeDeps.include + optimizeDeps.exclude to work around this, so for now we're using a janky PNPM patch 🙈. Is it possible to make the determination between in-repo and dependency code configurable?

Related

• Integration Vite into web apps in a "bit root" (@teambit) #15613

Reproduction

https://github.com/wburgin-anduril/vite-repro

Steps to reproduce

On the master branch:

• Run pnpm build
• Open bazel-bin/pkgs/foo-app/foo-app/dist/index.html and observe that the page works
• Run pnpm dev and observe the errors in the console

On the patched branch:

• Run pnpm dev and observe that there are no errors in the console and that the page works again
• Check out patches/[email protected] to see how Vite was patched on this branch

On the pnpm-dependencies-injected branch:

• Run pnpm to install dependencies
• Run pnpm dev and observe the errors in the console
• Take a look at how PNPM links @monorepo/foo-lib into @monorepo/foo-app in this configuration:

  ➜  vite-repro ✗ pushd pkgs/foo-app  
  ➜  foo-app ✗ node -e 'console.log(require.resolve("@monorepo/foo-lib"))'
  /Users/wburgin/Repositories/wburgin-anduril/vite-repro/node_modules/.pnpm/file+pkgs+foo-lib/node_modules/@monorepo/foo-lib/dist/index.js
  

System Info

System:
    OS: macOS 14.4.1
    CPU: (12) arm64 Apple M3 Pro
    Memory: 115.25 MB / 18.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.5.1 - ~/.nvm/versions/node/v20.5.1/bin/node
    Yarn: 4.1.1 - ~/.nvm/versions/node/v20.5.1/bin/yarn
    npm: 9.8.0 - ~/.nvm/versions/node/v20.5.1/bin/npm
  Browsers:
    Chrome: 124.0.6367.93
    Safari: 17.4.1

Used Package Manager

pnpm

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[alexeagle]

Bazel ecosystem maintainer here, +1 for a more principled way to distinguish external packages from those in the monorepo.

[hi-ogawa]

We haven't had any luck using optimizeDeps.include + optimizeDeps.exclude to work around this

In this specific case, I think using optimizeDeps.exclude for the packages which makes use of Vite specific features (in this case ?raw import) should work. Without this, optimizeDeps (i.e. esbuild) kicks in due to node_modules heuristics and it will try to bundle the code and end up with errors or something unexpected.

This might not be an ultimate solution for Bazel / Vite story, but I thought it's still worth mentioning. I verified that the following config works.

// pkgs/foo-app/vite.config.js
export default {
    optimizeDeps: {
        exclude: ["@monorepo/foo-lib"]
    }
};

In your reproduction, @monorepo/foo-lib is probably assumed to be consumed only inside own monorepo, but if it were something to be published as an independent package, then consumers would also need to deal with optimizeDeps.exclude for the package. This pattern is common when Vite plugin authors publish a package (thus ending up inside node_modules when it's consumed) but their feature requires Vite processing for their runtime code.

[bluwy]

What would be the suggested solution to distinguish between them? Different package managers can have different ways of linking. Checking node_modules is the fastest and often correct.

[wburgin-anduril]

Thanks for taking a look at this so quickly!

@hi-ogawa I think the issue we're having is that if we manually exclude our in-repo packages, Vite doesn't scan and process their transitive dependencies. I pushed a new optimize-deps branch which adds a react dependency to @monorepo/foo-lib and shows that if you optimizeDeps.exclude @monorepo/foo-lib you get an error like this in dev mode:

Uncaught SyntaxError: The requested module <omitted>/node_modules/react/index.js?v=b86f0c9c' does not provide an export named 'default' (at ShaderSource.js:1:8)

I think we would end up needing to manually add all of these transitive dependencies to optimizeDeps.include as well to make it work. At that point we're basically bypassing the heuristic entirely and providing the in-repo/dependency delineation ourselves.

---

@bluwy I was imagining something like letting the caller specify an optional callback in vite.config.js to differentiate between in-repo and out of repo dependencies, and falling back to the current node_modules heuristic when that callback is not provided. @alexeagle maybe you know of a better, more general way to differentiate?

[hi-ogawa]

I understand that the point you're making, but just in case, to provide more contexts, the intended usage is to write something like this https://vitejs.dev/config/dep-optimization-options.html#optimizedeps-exclude

    optimizeDeps: {
        exclude: ["@monorepo/foo-lib"],
        include: ["@monorepo/foo-lib > react"],
    }

Also there is a related issue / suggestion #16293 (comment)

[roysandrew]

+1 to this, particularly with things like https://github.com/tiktok/pnpm-sync or Rush's subspaces on the horizon it would be excellent if we could finally make workspace-enabled monorepos that make use of peer dependencies viable without significant hackery around vite.

The optimizeDeps include/exclude combo is something that I tried repeatedly to get correct but found significant issues with package deduplication, commonjs transpilation etc.

Would it be substantial overhead to enable plugins to augment this heuristic as a first step?

[bluwy]

I think it's unlikely for us to add a new option specifically to detect if a dependency is linked or not. Using dependenciesMeta.*.injected, or the file: protocol in pnpm, means that the dependency is hard-linked to node_modules, similar to how normal dependencies from npm are installed. The behaviour here had been relied on the ecosystem to mean "don't treat this as a linked package, but an npm package" and is not unexpected.

The pnpm docs also clarified this:

In contrast to normal dependencies, injected ones are not symlinked to the destination folder, so they are not updated automatically, e.g. after running the build script. To update the hard linked folder contents to the latest state of the dependency package folder, call pnpm i again.

Configuring optimizeDeps like @hi-ogawa suggested is the intended way to workaround this for now. However, I do agree that the monorepo experienced (or linked deps) isn't great, but we likely need to tackle this in a different way. This is currently tracked at #10447. Closing this for now.