fix: detect case-sensitive filesystems correctly under yarn PnP

[charlessuh]

yarn in PnP mode uses a virtual filesystem, and adding the preferUnplugged: true flag forces it to write this package to the real filesystem. This is needed so the case-sensitive detection logic in resolve.js works properly.

[ljharb]

Can you link to the documentation here?

This sounds like a bug in yarn PnP, to be honest; why should i add bytes to the package.json for all users, when yarn could fix it for yarn pnp users?

[codecov]

Codecov Report

Merging #2323 (369ee0a) into main (624aa61) will increase coverage by 0.55%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2323      +/-   ##
==========================================
+ Coverage   94.99%   95.55%   +0.55%     
==========================================
  Files          66       66              
  Lines        2699     2699              
  Branches      900      898       -2     
==========================================
+ Hits         2564     2579      +15     
+ Misses        135      120      -15     

Impacted Files	Coverage Δ
utils/resolve.js	99.15% <ø> (+11.01%)	⬆️
src/rules/no-unresolved.js	100.00% <100.00%> (+16.66%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 624aa61...369ee0a. Read the comment docs.

[charlessuh]

No, this is intentional in yarn: yarnpkg/berry#1762

Juat a little background: The virtual filesystem in yarn is an optimization, so when installing packages you are not creating a node_modules folder on the real filesystem via a gazillion filesystems calls. This field in package.json simply tells yarn to opt this package out of this, and extract this package somewhere on the real filesystem like npm would do. See:

• https://yarnpkg.com/features/pnp
• https://yarnpkg.com/configuration/manifest#preferUnplugged

Also, IMO a few dozen extra bytes in the package.json is pretty trivial. It's also arguably not unreasonable to decentralize this kind of data across potentially millions of packages, rather than centralize this kind of data in one place.

[ljharb]

On the contrary; the minority of yarn users are who should pay the cost, not everyone else.

[charlessuh]

Just out of curiosity (I’m not intending to defend yarn or fight any ideological battle here), but do you aim for this package to specifically only support npm, or does it aim to support as many users as possible given there are three package managers in the JS ecosystem (npm, pnpm, yarn)?

If this package only supports npm, then sure, even adding a single byte in this package to support any other package manager would be inconveniencing the only users of this package, who should be using npm.

[ljharb]

There’s one package manager that is the standard and has the majority, and whose behavior is correct by definition; I’m happy to support the others when it doesn’t hurt the primary audience.

If yarn’s virtual filesystem does not exactly mimic the native one, then it has a bug, and it should be fixed.

[charlessuh]

There’s one package manager that is the standard and has the majority, and whose behavior is correct by definition

If yarn’s virtual filesystem does not exactly mimic the native one, then it has a bug, and it should be fixed.

While I can appreciate that standpoint, I’m not sure how widespread this opinion that by /definition/ every single npm implementation detail is automatically standard and correct, and there is no room for package managers to differ.

If you take a look at the PnP page (https://yarnpkg.com/features/pnp#compatibility-table), you can see a large variety of packages like webpack, eslint, babel, prettier, gulp all have added explicit compatibility with yarn PnP.

In addition, the yarn team has stated this is currently intended behavior and not a bug (yarnpkg/berry#1762) and fixing it would be a lot of effort to fix a very small amount of cases like this.

I’m happy to support the others when it doesn’t hurt the primary audience.

Firstly, I’m open to alternative suggestions here. For example, instead of a metadata field in package.json, could this package add some JS logic to detect PnP via process.versions.pnp and do something else in that case?

Can you define what you mean by hurt? For example, would adding even a single byte to the download that would be unnecessary for a npm user be harmful?

[ljharb]

Certainly the mechanism in this PR is likely the least intrusive one available (altho yarn-specific metadata should really be inside a yarn object, instead of littering the top level with stuff)

Some of it is philosophical - just because some major packages have been effectively strong-armed into adding this metadata, doesn't mean that endorsing this dark pattern is something I want to do as a package author.

[charlessuh]

Okay. I'm trying to think of other ways to try to solve this, such as passing an additional explicit configuration option to tell this package whether the filesystem is case-sensitive or not (instead of having it detect it automatically), or perhaps using pkg-dir to detect the case-sensitivity of the project's package.json (which of course is on the real filesystem):

-const CASE_SENSITIVE_FS = !fs.existsSync(path.join(__dirname.toUpperCase(), 'reSOLVE.js'));
+const CASE_SENSITIVE_FS = !fs.existsSync(path.join(pkgDir.sync(__dirname).toUpperCase(), 'pacKAGE.json'));

[ljharb]

If package.json is always on the local filesystem, that seems like totally reasonable thing to check instead, if that bypasses yarn's bug.

[charlessuh]

I added an extra check in case pkg-dir returns null -- which I think might be possible if you have installed this package globally (https://github.com/sindresorhus/pkg-dir/blob/v2.0.0/index.js). I can confirm case sensitivity errors are being flagged for me on yarn.

[merceyz]

if that bypasses yarn's bug.

It's possible to mount the node_modules folder to a different drive using a different file system when using npm as well, shouldn't the check be testing on the file getting linted instead?

[ljharb]

Absolutely, that's a really good point. Want to make that change?

[charlessuh]

shouldn't the check be testing on the file getting linted instead?

Can you elaborate more on what you mean?

There are special-cases likepackage.json or files inside a published npm package where you could assume a correct case, but for a user-generated file, it could be named anything -- like MyRAnDoMFiLE.js -- and if we also check something like all uppercase, MYRANDOMFILE.js and that file also exists, it could be a case-insensitive filesystem, but perhaps that's a separate file that really does exist on a case-sensitive filesystem?

I'm also sure there may be operating system APIs that could do this, but does nodejs expose any of them?

[ljharb]

I don't think node has any API to expose that, unfortunately.

One technique would be to find a filename, and a variant, that don't exist - write one, and check to see if the variant suddenly exists - but this would just be another heuristic, not a guarantee.

We could use fs.readdir(path.dirname(currentFilename)) - and find a filename that exists, but who has a capitalization variant that does not exist - and then see if the variant exists directly?

[ljharb]

@charlessuh any interest in updating the check?

[charlessuh]

It looks like there is already logic in fileExistsWithCaseSync to query an expected filename, and see if the filename is the same case when you list the directory via readdir: https://github.com/import-js/eslint-plugin-import/blob/main/utils/resolve.js#L53-L56.

However, note this function returns early when CASE_SENSITIVE_FS is true. I've updated this PR to stop checking that variable -- so case sensitivity is now queried on a per-file basis.

[ljharb]

Thanks! I think this has evolved into a broadly useful fix - there's no reason any system's case sensitivity is constant for every file, since one file could be on a case sensitive volume and another on a case insensitive volume.

[ljharb]

@charlessuh hm, removing the utils change doesn't cause any of the tests to fail. Is there any way we could add a test for that?