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

custom path module #267

Open
mafintosh opened this issue Jan 7, 2022 · 15 comments
Open

custom path module #267

mafintosh opened this issue Jan 7, 2022 · 15 comments

Comments

@mafintosh
Copy link
Contributor

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?

@ljharb
Copy link
Member

ljharb commented Jan 7, 2022

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

@mafintosh
Copy link
Contributor Author

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.

@ljharb
Copy link
Member

ljharb commented Jan 7, 2022

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.

@mafintosh
Copy link
Contributor Author

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

@ljharb
Copy link
Member

ljharb commented Jan 7, 2022

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

@mafintosh
Copy link
Contributor Author

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

@ljharb
Copy link
Member

ljharb commented Jan 9, 2022

In sync, I see:

… 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.

@mafintosh
Copy link
Contributor Author

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.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

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)

@mafintosh
Copy link
Contributor Author

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

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

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

@mafintosh
Copy link
Contributor Author

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.

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

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, '/')}`?

@mafintosh
Copy link
Contributor Author

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

@ljharb
Copy link
Member

ljharb commented Jan 28, 2022

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?

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

No branches or pull requests

2 participants