-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
url.relative (to complement path.relative) #616
Comments
+1 from me. I see no reason not to have it. |
Not exactly a great rule to guide additions to core. I guess this points to the need for clearer guiding principles; something like "minimalism and quality API" would sum it up for me. |
Well, I don't mean we should just accept anything that's not blatantly against the spirit of core. Minimalism is certainly good. I just agree that the consistency with the path module is nice too, and it seems both generally useful and relatively easy to support. |
Seems like something that users would expect. ±0 |
Agree with @rvagg that there should be a guiding principle, and I generally agree with "keep core small, push things into userland modules." In that spirit, I published url-relative. Another principle to balance is "principle of least surprise" in terms of providing consistency across core apis. I knew about and use |
ftr I'm not a -1 on this, but I'm going to label it semver-minor so if this gets solid sponsorship from a collaborator and no -1s then it can be merged but we first need to figure out a good model for doing minor version bumps and handling that with git |
@jden maybe you could send a PR for it? |
There's also relateurl which does its best to keep the url as short as possible, which is pretty well the only reason to use a relative url. |
-1 core url library isn't the be-all of url manipulations, its not even spec conformant! See nodejs/node-convergence-archive#49 (EDIT: pointed to correct issue 49) url has minimal set of url features required to implement node itself, I don't think we should add to it, leave it to user-land. |
Yeah, closing unless people decide it's a good idea for inclusion. |
@sam-github #49 links to something else due to the split to the archive. See nodejs/node-convergence-archive#49 |
Imo this is a very core thing - just like URL parsing itself - to have in core. If resolving absolute URLs from relative URLs is considered core, then why is resolving relative URLs not? I've been using https://github.com/suarasaur/url-relative but it has some pretty serious bugs, like considering URLs with different protocols the same (e.g. |
@felixfbecker that is an awful library. Try "relateurl". |
@stevenvachon Thanks for the tip, that library definitely works better. However, it is clear that it was built for the use case in a minifier, and therefor has a lot of defaults I need to override to get pure relative URLs. For example, it removes default ports, removes trailing slashes, and returns absolute paths if the resulting URL is shorter. That seem like good choices for a minifier, but if I just want to create relative URLs for my application, and then for example mount the relative URL on top of a different URL with I still believe Node should have a low-level, unopinionated API for this in core. |
@felixfbecker the WHATWG |
The core
url
module currently hasurl.resolve()
to compute absolute URLs from relative segments (which is complementary withpath.resolve()
). but it is missing the complementary.relative()
function to compute relative URLs from two absolute URLs.Many programs simply use
path.relative()
when dealing with URLs (e.g.: https://github.com/substack/node-browserify/blob/ce3deecf7be27373c0131286bda86533ec83ffd1/index.js#L692 ) but on Windows systems,path.relative()
uses thepath.sep
character, resulting in relative URLs like..\\..\\a\\b
instead of../../a/b
.The algorithm for computing relative URLs is (AFAIK) the same, but the separator character should always be a
/
as specified in RFC 3986 Section 3.3.Opening this for discussion here prior to writing a PR.
The text was updated successfully, but these errors were encountered: