resolve icon indicating copy to clipboard operation
resolve copied to clipboard

custom path module

Open mafintosh opened this issue 2 years ago • 15 comments

Hi! We have a use case where we'd like to do our own resolve,dirname,join logic. We have a (crude) workaround for now, would you be interested in a PR that adds a path option like we can do we the fs ops?

mafintosh avatar Jan 07 '22 08:01 mafintosh

Can you elaborate on that option and what you’d like it to do? We already have a pathFilter option.

ljharb avatar Jan 07 '22 16:01 ljharb

We're doing resolve but using urls and not files, so for example, we use our own dirname (that is not platform dependent) ie

resolve('mod', {
  basedir: 'http://foo.com/......./',
  readFile (url, cb) {
    getUrl(..., cb)
  }
})

Now with the current resolve, it's super easy to add all the custom file io, except for how to join paths - there it's currently hardcoded to how path does it "path". Currently we work around that, by doing

resolve('mod', {
  ...
  readFile (url, cb) {
    // try to "recovery" from path.join into a url again
    url = unpathish(url)
    getUrl(..., cb)
  }
})

But that obvs has a ton of edge cases, all easily solved if we could just pass our own path impl.

mafintosh avatar Jan 07 '22 16:01 mafintosh

I don’t think it makes any sense to use this library for URLs; file paths aren’t URLs, and resolve does lots of filesystem interaction. The only way to resolve a URL is to fetch it.

ljharb avatar Jan 07 '22 16:01 ljharb

Yea, I can abstract all of that already using the readFile hooks etc, just need a similar hook for path

mafintosh avatar Jan 07 '22 17:01 mafintosh

So which specific thing are you trying to hook into? The path.join calls?

ljharb avatar Jan 07 '22 19:01 ljharb

All the stuff that happens with path, ie resolve(..., { path: myOwnPath })

mafintosh avatar Jan 09 '22 20:01 mafintosh

In sync, I see:

  • path.join:
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L55
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L138
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L161
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L195
  • path.dirname:
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L78
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L109
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L141
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L204
  • path.resolve:
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L84
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L93
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L115
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L181
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L186
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L189
  • path.relative:
    • https://github.com/browserify/resolve/blob/main/lib/sync.js#L112

… which is a pretty massive amount of surface area.

Can you help me understand more about the use case? Browsers and deno deal with URLs, node really only deals with file paths despite the ESM subsystem converting everything to URLs (altho it may, sadly, accept non-file URLs in the future). "resolving" only really conceptually makes sense on a filesystem, or with import maps; with URLs, the server must be in control of resolution, and that can't be encoded in a library.

ljharb avatar Jan 09 '22 21:01 ljharb

I think this should be as simple as { path: myPathModule } and in resolve you just do const path = nodePath || opts.path.

The usecase is what I describe above - We use urls in electron to describe content and use resolve to implement require in that system, so we need platform agnostic resolution.

mafintosh avatar Jan 27 '22 10:01 mafintosh

Right, but resolve is fundamentally not designed to be used with URLs, it’s designed for filesystem paths - so I’m not clear on why it would make sense to provide a hook to change how paths are combined (especially not one entire hook for the path module, which contains implicit assumptions about how things work that would be dangerous and laborious to try to enumerate)

ljharb avatar Jan 27 '22 14:01 ljharb

The url path is not important. If I could just lock it to use the unix path module always, that works as well. I just need it to use /a/b/c paths always disregarding the os, instead of c:\a\b\c when run on windows

mafintosh avatar Jan 27 '22 15:01 mafintosh

ahhh, thanks, that does clarify a bit. Is replacing path.sep with / not an option?

ljharb avatar Jan 27 '22 15:01 ljharb

I have a workaround that does all kinds of things like that, but it's hacks. Replacing the sep turns c:\a\b\c into c:/a/b/c so you need to sniff for windows roots as well. Much cleaner if I just didn't have to undo the work.

mafintosh avatar Jan 27 '22 15:01 mafintosh

hmm - from a windows absolute path, if you only care about relative paths, would it not be `./${path.relative(result, process.cwd()).replace(path.sep, '/')}`?

ljharb avatar Jan 27 '22 20:01 ljharb

No, cause the files are resolved in a “virtual fs” using the readFile etc options. In anycase this issue was to support a simple thing to make it platform agnostic, I have a workaround. Happy to PR it or feel free to close this issue if not interested

mafintosh avatar Jan 28 '22 04:01 mafintosh

I mean, I'd like to find a way to make what you need to do easier, without drastically widening the API surface of resolve.

resolve is already platform agnostic by its use of the core path module - it's that electron is trying to use it for URLs in a way it's not intended to be.

If the files are resolved in a virtual filesystem, could - on windows - the virtual filesystem use backslashes?

ljharb avatar Jan 28 '22 05:01 ljharb