-
Notifications
You must be signed in to change notification settings - Fork 43
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
updated dependencies, added trailingSlash option #19
Conversation
Agreed. Closing #17 for this. The tests worked seamlessly, but my solution might run into some some issues with edge cases later on. |
I'd be more happy with ability to configure options globally. For example: urlJoin.config({
trailingSlashes: true
})
urljoin(...) // -> will put trailling slashes However, particularly in described case it's is unclear why would we need this option at all. In other words. in this example
output is wrong not because of lack of option, but because of urljoin wrongly handles hashbangs during escaping. '#' for some reason not treated as an url fragment, while it should, because it is just an explicitly passed string. I can not imagine situation, when you would not want slashe before So, I'm against this PR until more real usecase will be shown. But I'm 👍 for fixing issue with hashbang. |
I agree with @ArmorDarks |
@ArmorDarks The most basic usecase would be |
In such case you should write not this urljoin('https://example.org', 'whatever', '#', 'headline2') because it should produce Since library can't guess did you want it to be be segmented, or urljoin('https://example.org', 'whatever', '#headline2') Note how you specify This should produce
Particularly your use case is a bit trickier, because it contains link to page, and we indeed can't add trailing slash here. So this urljoin('https://example.org', 'whatever.html', '#headline2') Will result in invalid url I think it is possible to avoid by looking for segment before Hm, though, it might be tricky because of possible domain name in previous segment. I really start to think that merging hashes and queries this way isn't good idea at all, and that they should be passed as object with parameters, like { hash: heading2 }. Then it will be easier for lib to construct proper url |
Are there plans to merge this? We are stuck with an issue that will be fixed with this PR. We need this library preserves the last trailing slash before the query string. |
@mangelsnc read my post above. Maybe we can think out better solution. I don't think that rushing with adding new options without considering all other solutions is good idea. It's like patching huge hole with thin paper. |
Updated Dependencies: the dependencies for testing (mocha, should) were out of date. I updated them which also makes test output much nicer.
added trailingSlash option: Using url-join with the arguments of an array of url parts and an object for settings now allows to set the
trailingSlash
option, which will keep slashes in front of#
and?
instead of removing them.Sample: