pathFilter (or some new API) should support implementations of exports

[SimenB]

In lieu of this module supporting exports out of the box, it would be awesome if consumers could use its existing capabilities (packageFilter, pathFilter etc.) to implement support for exports on the consumer side.

I have experimented a bit with this, and from what I can tell there are 2 issues:

1. resolve adds index if a directory is specified (

   resolve/lib/sync.js

   Line 192 in f1b5184

   return loadAsFileSync(path.join(x, '/index'));

   ). This can potentially cause issues in the case index is not specified by the user as changing it to . (which is what a directory means in exports) is not necessarily a safe operation. Not sure how to mitigate this in a way that's backwards compatible.
2. I tried to use pathFilter which works great for require('some-module/thing'), but it doesn't work for require('./some-thing') from within a module. Relative imports within a module isn't restricted by exports, but pathFilter cannot know if the resolution request is for a relative file or not. A solution here is maybe to include a flag saying if it's relative within the package or not?

[ljharb]

That would be great to enable, until we add support natively.

1. "." in exports only refers to the "main", but not other directories - but you're right that with exports, there's no auto-index lookup. I'm not sure how to handle that without adding a whole new flag (that would be obsolete once exports support is added).

2. pathFilter receives three arguments; the second should be the full path - relativePath is the one that has the leading dot, no?

[SimenB]

1. True about it becoming obsolete once native supports lands, but it could be removed in a v2 or something? And the logic itself would be kept, just no way of manually switching it on or off

2. pathFilter - no leading dot, so it has no way of knowing if the request was require('module/file') or require('./file') from within module/index.js for instance. The relativePath arg passed in this case is just file. It could be ./file for module relative imports, and file for "external"? That's breaking tho, so for backwards compat I guess we'd need a new arg or something saying if it's one or the other. Or possibly, since resolve only works on a single from and to at the time, I could look at the basedir that I pass, and based on that differ in the logic? That might work 🙂

[ljharb]

resolve is ubiquitous enough that i'll be maintaining both v1 and v2 (which is already out in prereleases, but could still gain breaking changes) for the foreseeable future. The main issue is that if in v1, you disabled the lookup and also enabled the new "exports" logic, we'd have to throw.

Certainly if you have enough information already, that would be ideal :-) making pathFilter prepend the leading dot for relative paths seems like a great v2 change tho, if you wanted to make a PR (v2 is master, v1 is 1.x, ftr)

[SimenB]

Ok, gave it a whack over in jestjs/jest#11961

1. I check if relativePath given by resolve is 'index' and if the original request did not end with index, I translate that to ..
2. I don't look at exports at all if the require is relative (starts with '.') or it's an absolute path.

Not 100% if that's correct, but it seems to work... 😅

[ljharb]

Seems fine - part of the reason resolve's implementation has taken so long is that without gobs and gobs of test cases, it's hard for me to be confident I've gotten it right (and almost every time i've added a fixture project, i find a way i got it wrong) - so you may want to look into adding lots of tests :-)

[SimenB]

Right, it breaks a test, but I think that test is faulty (it imports a "local" module with a package.json with exports via a relative path, but that is not in node_modules). I think that's just wrong behavior, so it failing now is actually a bug fix and my old test was just bad 😅

Might just roll this out behind a config flag and hope people test it and report instances where it behaves wrong (after adding more tests)

[SimenB]

pathFilter is working out well 👍

Would be nice to have some way of disabling main and the index.js in root stuff if there is exports, but I guess doing that in isolation doesn't make much sense. And ignoring it manually isn't too bad

[ljharb]

Once we have true "exports" support, it'll take precedence over the "main" anyways, so you won't need to ignore that.

[SimenB]

Yeah, figured it'd make more sense to go straight there instead of some intermediary step 🙂

[SimenB]

Hmm no, hit a case I forgot to add a test for 😀 Of course...

See jestjs/jest#12373, any help appreciated! 😀 Happy to send a PR to this repo if we figure out how to solve it. I think resolve should walk up the FS until it hits a package.json in a directory, then call the filter functions, then check if it exists. Not 100% sure, tho 🙂

[SimenB]

Ok, closing again 🙈 My solution is to pop of anything trailing after pkg/ or @scope/pkg/, add ./package.json and resolve that using resolve, and if package.json has exports resolve those with anything trailing (or . if nothing), and return the full path. No packageFilter or pathFilter at all. In theory a tiny bit faster as well as we head straight for the package.json file and potentially shortcut instead of traversing and checking for file existence.

(code is in above PR if this explanation made no sense, which it probably doesn't)