Introduce treatAsNonHtml to remove isHTML implementation
#1230
Conversation
|
I'm wary of using an exclusion list and assuming that most things are HTML by default. I think the list will just keep growing. But I do like the API you've proposed better than Turbo.htmlExtensions.add(".foobar")The implementation would be as you've proposed, but would check for inclusion in the HTML extensions, rather than exclusion. We'd ship with the defaults already defined by the regex: export const htmlExtensions = new Set([ ".html", ".htm", ".xhtml", ".php" ])
export function locationIsVisitable(location, rootLocation) {
return isPrefixedBy(location, rootLocation) && htmlExtensions.has(getExtension(location))
}EDIT: I guess that won't work when there's no extension – the implementation would need to account for that. |
|
On the one hand finding the extension is always a educated guess. In our application we have URL's with a filter, containing a date range presentation. Example: On the other hand, we need to think about fallback situations. If the So, I'm still in favor of the proposed set for non-HTML extensions. I'm aware it is not covering all possible extensions, but when it does not cover an extension, the user won't notice and the experience stays the same. I like the |
Thanks for the explanation. I hadn't considered dynamic paths like the one you described. I think your reasoning makes sense. I think we'd need a much larger default exclusion list, however. Just looking at Basecamp and HEY, we have a pretty extensive list of extensions we'll auto-link to: zip, tar, gz, bz2, rar, 7z, dmg, exe, msi, pkg, deb, iso, bmp, mp4, mov, avi, mkv, wmv, heic, heif, mp3, wav, ogg, aac, wma, webm, ogv, mpg, mpeg. While these could return HTML, it's extremely unlikely, and we wouldn't want to start making two requests to resolve them. |
|
Thinking about it more, I think there are more downsides to an exclusion list than the inclusion regex we have now. It makes more sense to define the extensions that we're confident are HTML than it does to define the ones that aren't. The former is going to be a much smaller set than the latter for all practical purposes. The example with a pathname like |
|
For the inclusion list/regex to work, it should have perfect coverage. My situation is very specific, but there have been plenty of other examples like FQDN like example.com, Rails ids in paths, .deploy, file extensions, and usernames with a dot in it. I don't see how a default list/regex is going to cover all these situations. If the list/regex is a configuration, a user could extend the list and cover their needs. But even then it is hard to get it covered. Many paths contain dynamic parts that are hard to contain in a list or regex. But I'm also thinking about the developer experience. A new developer starting with Turbo expects Turbo to work all the time. When a part of the requests/refreshes are suddenly page reloads, it is not clear at first hand what is happening. This was my firsthand experience last few days when users complained about random refreshes and we couldn't find the cause because the I don't see a downside for a long(er) exclusion list. We probably can find a reasonable list of say 100 most used file extensions on the web that give no HTML response. If we miss something, the end-user won't notice other than the double request. And the developer can safely extend the list by providing a specific extension. What I was thinking about, is the introduction of the (first) Turbo configuration. Instead of going with the |
|
Is it too complicated to add a slash at the end? In Rails you can easily do it with the |
Yes, this has all kinds of side effect in our application. Furthermore I believe a library like Turbo shouldn't dictate if slashes are added or not. Rails might have an easy solution for this, but isn't this library intended to be used by other frameworks too? |
|
How are we going to move forward with this PR? I haven't read any arguments that have convinced me the exclusion list is not going to work, other than some minor inconveniences with maintaining the list. In my point of view this downside does not outweigh the downside of unexpected behavior with full page refreshes due to some random |
Fixes #608
When Turbo navigates to a new URL,
locationIsVisitableis called to check if the location is visitable. One part of the check is checking if the requested URL returns HTML. We can never determine with 100% confidence if a request will return HTML. A page with a.htmlextension might still return anothercontent-type. A page with a.foobarextension might return HTML. Using a check on the file extension is therefore imprecise and prone to bugs.Turbo already handles non-HTML responses. When a response doesn't contain an HTML part, a
contentTypeMismatchis raised. This causes the page to reload to the requested URL. So we could remove theisHTMLcheck completely and still have a functioning situation. The major downside is having two requests: one Turbo fetch and one regular browser request.A user could prevent these double requests by adding
data-turbo="false"to these links. But there might be some sensible default by never handling known non-HTML extensions as a Turbo request. This PR introduces theTurbo.treatAsNonHtmlsetting to enable this.By default, a range of common non-HTML extensions are included in this setting, like .png and .jpg. If users have many links to other extensions and don't want to sprinkle their HTML with
data-turbo="false"attribute, they can add new extensions to the set:Because these changes are added to the lower-level checks, they work both for regulars visits and the newly introduced page refresh morphs.
Mentioning @packagethief @dhh @jorgemanrubia for some visibility.