added trailingSlashes option #31
Conversation
|
Seems like something that can be added. Can you make the setting apply to the text and JSON views as well? |
|
I can, when i find the time. I'll try to do it before the weekend is over. |
|
Hi Doug, I can't seem to find how i would change and test the JSON and plane text use-cases. Could you help me out with it? Thanks |
|
No problem. If you have any work in progress, feel free to push it up and I can check it out/help/just get it down for you :) |
|
I changed enough for it to work like i need it, (lines 395 - 398) but i can't see where i could change it for the JSON and text views... |
|
No biggie, Ill look into it :) Once July comes, this module finally won't be tired to Express 3.x release cycles, so we'll be able to release faster :) Otherwise right now a new feature just waits for the next 3.x minor, which is why there hasn't been too much of a rush (sorry!). |
|
no worries, we're already using it without a problem. Thank you for doing the extra work, but i really couldn't find the place where to edit. |
I wasn't aware the last comment I had made was ever addresses, sorry! Github doesn't email when PR commits are added. |
|
Oh sorry, I didn't realize that! I wonder if @andrepadez could review my PR and see if it covers what he needs? |
|
Guys, sorry but i haven't had the time to review your PR. I'm going to do that now. |
|
@geekmug , added a comment to your pull request. Other than that, i question @dougwilson if adding the trailing slash indiscriminately, won't affect currently dependant projects... |
|
Yes, to add something like this we need an opt in flag until the next major. Now that express 3 has an end date (July), that's when a lot of these module can finally bump their major versions! |
|
So yeah, i don't mind @geekmug's pull request to be merged instead of mine, but these issues need addressing. Thanks again, for now i will keep using my fork. |
|
From my perspective the current behavior is a bug, since serve-static will cause a redirect. I don't think I should have to opt out of having a bug. I can't imagine a use case for the HTML output that doesn't involve serve-static, and I can't imagine a use case that involves mangling the strings from the plain or json APIs either. However, my original PR was for the HTML output only because of the obviousness of this being a bug. If the concern about breaking someone else's public API (where someone is passing through the output of either the plain or JSON output), then leave that part out (or make it optional), but I'd be interested to see evidence of that. |
|
@geekmug I published this change in a patch long ago and got enormous backlash. This change will either need to live behind a opt-in switch until 2.0 or not go in at all until 2.0. Those are the only options I can offer, unless you offer to field all bug reports for me. |
|
Though even if you do offer, I probably still won't, because our users are expecting this behavior that has existed since at least 2013 (remember, this was extracted from connect 2.x and 1.x of this module is used in connect 2.x/express 3.x) to keep. Some may have added a hacked workaround we would break into their code that is so old it may no longer even have an owner. This is the stability guarantees expressjs promises. I don't disagree that having no trailing slashes doesn't make sense, hence the reason I had previously released a patch to fix it, but it broke too many people that it's being held until the next major (or opt in via switch). |
|
Hah, I must not be creative enough to imagine how people have abused this module. I am comfortable with you rejecting my PR on that basis. |
|
I'll figure out these two PR statuses soon (likely this week, as a new minor of connect and express is scheduled, which is when all these modules incorporate all their minor updates like this). |
|
I am happy to build on what I've done and apply it to the Json and HTML templates now I've seen how Sean has done it. Cheers Sent from my iPhone
|
@dougwilson, @geekmug, is it ok with you guys? |
I came to a situation where i need the links to directories to include the trailing slash.
so, when i click in
some-directoryit will actually link to/path/to/some-directory/instead of/path/to/some-directory.It is not intrusive, i added it as an option, and it still defaults to false so it doesn't break anything
Thanks