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

Doesn't resolve modules with pnpm #67

Closed
timsuchanek opened this issue Apr 17, 2020 · 8 comments
Closed

Doesn't resolve modules with pnpm #67

timsuchanek opened this issue Apr 17, 2020 · 8 comments

Comments

@timsuchanek
Copy link

pnpm is not using hoisting, but always nests the node_modules.
It seems like this doesn't work with esbuild.
Here is a simple repro: https://github.com/timsuchanek/esbuild-pnpm

@evanw
Copy link
Owner

evanw commented Apr 18, 2020

Caveat: I'm totally unfamiliar with pnpm.

Thanks for the simple repro case! It was very straightforward to reproduce the issue.

I'm just reading up on this tool now. I noticed that the FAQ (https://pnpm.js.org/en/faq) says this:

pnpm does not work with <YOUR-PROJECT-HERE>?

In most cases it means that one of the dependencies require packages not declared in package.json. It is a common mistake caused by flat node_modules. If this happens, this is an error in the dependency and the dependency should be fixed. That might take time though, so pnpm supports workarounds to make the buggy packages work.

...

Solution 3

In case there are too many issues, you can use the shamefully-hoist config. This creates a flat node_modules structure similar to the one created by npm or yarn.

To use it, try pnpm install --shamefully-hoist.

I can verify that if you follow your instructions and do pnpm install followed by pnpm run build then the build fails. These are the specific errors:

node_modules/chalk/source/index.js:2:27: error: Could not resolve "ansi-styles"
const ansiStyles = require('ansi-styles');
                           ~~~~~~~~~~~~~
node_modules/chalk/source/index.js:3:59: error: Could not resolve "supports-color"
const {stdout: stdoutColor, stderr: stderrColor} = require('supports-color');
                                                           ~~~~~~~~~~~~~~~~

However, if I do pnpm install --shamefully-hoist followed by pnpm run build then everything works fine.

Does this mean that, as the FAQ says, this is an error in the dependency? Sorry if that's not the case. I've never used pnpm before.

Also: Is there a description of the changes to the module resolution algorithm that I'd need to use to support pnpm? Node has a great reference for its module resolution algorithm, for example: https://nodejs.org/api/modules.html#modules_all_together.

@timsuchanek
Copy link
Author

Thanks for the answer @evanw!
To be honest, I also don't understand enough, why the Node.js module resolution works in my example and what exactly pnpm is doing under the hood. I pinged the author of the library to give us an explanation here about how the module resolution algorithm would have to be adjusted in esbuild.

The --shamefully-hoist is not an option for us, because it slows installation down (we have a big workspace) and we have globally conflicting TypeScript types (of jest and mocha), which would conflict with hoisting, but are not conflicting with the pnpm module isolation.

@zkochan
Copy link

zkochan commented Apr 20, 2020

@evanw seems like the issue is that esbuild does not use the real location of the files when resolving dependencies. The real location of chak is not at node_modules/chalk. The real location is at node_modules/.pnpm/registry.npmjs.org/chalk/4.0.0/node_modules/chalk.

node_modules/chalk is a symlink to node_modules/.pnpm/registry.npmjs.org/chalk/4.0.0/node_modules/chalk. Any bundlers should resolve modules to their real location and resolve dependencies from that location (this is how Node's resolution algorithm works). Dependencies of chalk are at node_modules/.pnpm/registry.npmjs.org/chalk/4.0.0/node_modules/

@evanw
Copy link
Owner

evanw commented Apr 21, 2020

That's very helpful! It'll probably be a little tricky to fix this without regressing performance. It looks like the naive approach is a big performance hit. I'll have to figure out a caching strategy.

It seems like this doesn't work in Parcel either. I believe parcel-bundler/parcel#1125 is the corresponding issue.

@evanw evanw closed this as completed in b1555be Apr 21, 2020
@evanw
Copy link
Owner

evanw commented Apr 21, 2020

Just gave fixing it a shot. The module resolution algorithm should now return the real location instead of the symlink location. Please let me know if that works for you or not.

@timsuchanek
Copy link
Author

The fix works! Thanks a lot!

@SevereOverfl0w
Copy link

Unfortunately, this does break npm install to a directory, which creates a symlink. The parcel issue also discusses this. Their solution to this is to act differently when the package.json contains "source": true.

@evanw evanw mentioned this issue May 26, 2020
@jackyef
Copy link

jackyef commented Jul 21, 2020

Hi @evanw , I just tried this on a windows machine (0.6.5, installed from npm) but the build fails with the same error as described in your earlier comment. Has this regressed since then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants