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

Bug: resolves using node_modules when using PnP #273

Open
merceyz opened this issue Dec 22, 2020 · 12 comments
Open

Bug: resolves using node_modules when using PnP #273

merceyz opened this issue Dec 22, 2020 · 12 comments

Comments

@merceyz
Copy link
Contributor

merceyz commented Dec 22, 2020

Describe the bug

When running under Yarn PnP enhanced-resolve will still use the node_modules resolution

To Reproduce

Setup:

mkdir repro && cd repro
yarn init -y
yarn set version berry
yarn add [email protected] [email protected]

Code:

import { ResolverFactory, CachedInputFileSystem } from 'enhanced-resolve';
import path from 'path';
import fs from 'fs';

const cachedFS = new CachedInputFileSystem(fs);

const commonResolver = ResolverFactory.createResolver({
  fileSystem: cachedFS,
  useSyncFileSystemCalls: true,
});

fs.mkdirSync(path.join(__dirname, 'node_modules/lodash'), { recursive: true });
fs.writeFileSync(path.join(__dirname, 'node_modules/lodash/foo.js'), '');

commonResolver.resolveSync({}, __filename, 'lodash/foo');
throw new Error(`Resolve should have failed since lodash/foo doesn't exist`);
@merceyz merceyz changed the title Bug: incorrectly resolves using node_modules when using PnP Bug: resolves using node_modules when using PnP Dec 22, 2020
@alexander-akait
Copy link
Member

Expected, because modules: ['node_modules'] by default, set modules: []

@merceyz
Copy link
Contributor Author

merceyz commented Dec 22, 2020

I originally did that, but that causes it to throw on resolving commonResolver.resolveSync({}, __filename, 'lodash');

@alexander-akait
Copy link
Member

What is the real case problem?

@merceyz
Copy link
Contributor Author

merceyz commented Dec 22, 2020

I'm adding exports support to the PnP api so I need enhanced-resolve to only do the resolution from the result of resolveToUnqualified, when resolveToUnqualified returns a path, if the file enhanced-resolve is looking for doesn't exist it will keep on climbing the filetree and potentially return a path from anything else

@alexander-akait
Copy link
Member

Please describe the problem, not code, exports in package.json?

@merceyz
Copy link
Contributor Author

merceyz commented Dec 22, 2020

The problem is that enhanced-resolve walks up the filetree and out of the node_modules folder given to it by PnP

exports in package.json?

Yes, the pnp patch doesn't support it at the moment, it only respects the main field

@alexander-akait
Copy link
Member

The problem is that enhanced-resolve walks up the filetree and out of the node_modules folder given to it by PnP

So you expected throw error?

Yes, the pnp patch doesn't support it at the moment, it only respects the main field

It was fixed in the latest version, webpack supports exports for PnP

@merceyz
Copy link
Contributor Author

merceyz commented Dec 22, 2020

So you expected throw error?

Yes, it has been told where the package is but the file (foo.js) is missing so it should throw.

It was fixed in the latest version, webpack supports exports for PnP

I'm aware, it's for the pnpapi itself, so that it's supported when running require and require.resolve in node
yarnpkg/berry#650

@merceyz
Copy link
Contributor Author

merceyz commented Dec 29, 2020

I'm aware this is how the node modules resolution works but would you be open to changing it for when PnP is enabled / adding an option for it? PnP is supposed to not let you resolve something you haven't declared but enhanced-resolve currently allows that to happen, i'm able to workaround this in Yarn so it's not critical but would be nice to avoid all that extra IO only to reject the result

@noahnu
Copy link
Contributor

noahnu commented Mar 30, 2021

I've run into this on a project and spent quite a bit of time confused as I wasn't expecting webpack to fallback to node_modules. In my project, this has caused 2 different instances of react-dom to be loaded, one from pnp and one from node_modules, resulting in react crashing.

I'm still trying to figure out why webpack is falling back to node_modules, since I'm not seeing any errors. Would definitely be nice if webpack exclusively used pnp when resolving dependencies in a pnp project.

@jgornick
Copy link

@merceyz I'm trying to follow along in this older issue you created, but does #301 help with this original issue?

@merceyz
Copy link
Contributor Author

merceyz commented Nov 11, 2022

No, it shouldn't affect this issue.

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

4 participants