deprecate / metal-uri

Class for parsing and formatting URIs.
Other
3 stars 7 forks source link

Trailing slash being removed #3

Closed zenorocha closed 6 years ago

zenorocha commented 8 years ago

When passing a string like /path/ this component is removing the trailing slash to /path.

new metal.Uri('/path/').toString() === '/path'

This can cause errors when using Senna depending on how Apache/Nginx is configured to handle URL redirects.

graywolf336 commented 7 years ago

Any chance I can get an update on this? We are trying to use senna.js on Rocket.Chat's Documentation pages however we ran into https://github.com/liferay/senna.js/issues/131 which was causing our images not to load https://github.com/RocketChat/Rocket.Chat.Docs/issues/95. Our temporary solution, and rather an ugly hack, was to use jquery to set the <base href=""> each time the user clicks to go to another page.

mairatma commented 7 years ago

Hey @graywolf336, we haven't done this yet because it would be a breaking change that could cause other problems inside senna.js, as well as in other projects using metal-uri.

I've closed this issue you've mentioned from senna.js explaining that my problem with images wasn't really due to the slash, it was actually expected browser behavior due to the missing <base> tag in my html. In my case I just added it once there and it was enough to fix everything. As long as this tag is added to the every page's header in the server it should be fine, so you wouldn't need to do it on jquery.

graywolf336 commented 7 years ago

@mairatma Ah okay.

Yeah, your closing comment lead us in the direction to get it fixed however adding the <base> tag to each page didn't work with senna.js as it wasn't changing the <base> tag value on each page change. So that's why we had to end up going the jQuery route.

mairatma commented 7 years ago

Usually <base> should have the same value on all pages, since it just points to the path in the server where relative requests should point to. So as long as your image srcs are all relative to the same value, <base> can be the same for all pages. Maybe some of your pages are using different relative paths? If can you fix that you'd probable be able to remove the jquery code for this.

graywolf336 commented 7 years ago

Yeah, that's our problem. We are using senna.js for our documentation and using metalsmith to generate the html from our markdown files which we have a boat load of them. The images that are relative are stored inside nested folders, so they're not all served from the same place (see here and here).

mairatma commented 7 years ago

Are they being served from the same server? If so you could set <base> to some common root between them, like this Rocket.Chat.Docs folder, then you could use relative paths like this: /4.%20User%20Guides/leave-icon.png and /1.%20Contributing/Donating/coinbase.png.

graywolf336 commented 7 years ago

Yes they are, but each item is transformed into a "folder" and served as static content. Then behind the scenes with nginx we rewrite if it is a folder/directory and if it is a file. That's why the images are all relative in the same folder. It's overly complicated and I'm not 100% sure why they went this route, but it's working with a few "workarounds" via jquery. Once we improve the transformation code, then we'll hopefully be able to support one <base> value instead of having to change it.

mairatma commented 7 years ago

Got it, I'm sorry I couldn't be of more help. Unfortunately even if this issue was implemented in metal-uri you'd still have the same problem. We thought it was related but it wasn't in the end :(

graywolf336 commented 7 years ago

Thank you for trying to help, hopefully in the near future I can come back and say everything is working perfectly without any workarounds. 👍

jbalsas commented 6 years ago

Hey @zenorocha, @eduardolundgren, is this still a concern? Should we try to fix it? We might try to get it in for 3.0.0, but it looks like this would be a breaking change...

@Robert-Frampton?

robframpton commented 6 years ago

@jbalsas

Since this would be a breaking change, and Senna has most likely been fixed with a workaround, I think we should just close it.

jbalsas commented 6 years ago

Let's close it then and see if this pops up again with more usage.