Turbo will not navigate to URLs that have a . in the pathname

[manuelpuyol]

turbo/src/core/url.ts

Lines 27 to 29 in 98cdc40

	export function isHTML(url: URL) {
	return !!getExtension(url).match(/^(?:|\.(?:htm|html|xhtml))$/)
	}

It looks like that this behavior is intended, guessing it prevents Turbo from running when users are downloading a file. The problem is that routes with . are valid in Rails routes, so, if you tried to navigate to routes like:

• https://github.com/hotwired/turbo/releases/tag/v7.1.0
• https://github.com/hotwired/turbo/blob/main/CONTRIBUTING.md

Turbo won't even try to navigate to them.

[dhh]

@packagethief You just looked at this, right? We're doing this to prevent Turbo from trying to load .jpg and .mp4 etc.

[dmcge]

There’s some related discussion here: #519

[tleish]

see also:

• Links with a dot are not processed by turbo #385
• Turbo is ignored on any navigation that uses a dot in the URL #480

[manuelpuyol]

thanks for the context!
@packagethief any plans on moving forward with #519 ?

[packagethief]

Yes, we discussed beginning in #519 (comment) and the conclusion was that we should introduce API so applications can customize.

We just need to decide on what that API should look like. Originally I'd imagined hanging something off the Turbo global, but we don't have precedent for that. It also seems like it will lead to unwieldy heuristics as applications try to encapsulate all possible paths.

I'm liking the suggestion in #519 (comment), which is that we assume all URLs are turbo-visitable by default, and require applications to annotate the ones that aren't. Turbo will already avoid following links annotated [download], and this feels conceptually similar. We could suggest using data-turbo=false for this, which would already Just Work, or we could introduce a new annotation like type. I can't see a reason to introduce a new attribute when data-turbo already exists.

[dhh]

Okay. That is a breaking change, though. If suddenly links to a jpg that were working fine before stop working unless you annotate. Maybe we could do both? Build a type list that we'll default to off on (all image + audio + video types). Then everything is default on, and you have to opt out beyond that?

[manuelpuyol]

I don't think it's a breaking change since the idea is to provide a way for users to customize what is visitable or not. The default will be the same, and each app can override it if they want to
Simply updating the turbo version won't break anything for users

[packagethief]

Yeah, we need to be backward compatible, so we can't change the default behavior.

I think the simplest thing to do is expose something on Turbo.session. The raw implementation of isHTML() isn't a very friendly thing to override though. That's what I think could get unwieldy. Maybe something more granular, that passes the extracted extension? Turbo.session.extensionIsHTML(extension).

[packagethief]

This is an example of what I consider unwieldy, just to quantify it:
https://discuss.hotwired.dev/t/how-to-make-turbo-drive-work-with-php-files-and-query-strings/2020/2

This is too onerous for official API. Now that I think more about it, the lack of a good idea for appropriate API is the main reason we didn't add this years ago. So, suggestions appreciated!

[manuelpuyol]

I do like the idea proposed in #519 , where they are exposing a isVisitable function that can be overridden. Maybe it could be improved to make the function receive the extension instead of full URL like you mentioned, but it allows Turbo the keep the default and allows users to change it as they seem fit.

[f6p]

I don't think we need new API. We could just extend URL and use it internally. Then it will be passed to isVisitable. Obviously first thing to implement in inherited class would be extension method.

[f6p]

Or even simpler than that. Pass basic object to isVisitable with attributes reassigned from URL and some extras (extension etc).

[f6p]

Or alternatively keep extensions in their own separate array attribute. So if certain extension should be turbo-enabled just push it there. And if more flexibility is needed reassign isVisitable.

But is it really needed?

let components = testing.pathname.split("/").slice(1);
let lastPathComponent = components.slice(-1)[0];
let extension = (lastPathComponent.match(/\.[^.]*$/) || [])[0] || "";
    
return location.origin === testing.origin && !!extension.match(/^(?:|\.(?:htm|html|xhtml|aspx))$/)

can be written as just:

let parts = url.pathname.split('/').pop().split('.')

return (parts.length > 1) && ['doc', 'txt'].includes(parts.pop())

With better defaults (addition of php for example) I think it may not be worth the effort and simple function would be more readable.

[2called-chaos]

I would like to add that it would be nice if data-turbo=true does what it says, I'm not sure #519 is doing that (but I could be mistaken).

Global config is nice and all for PHP folks, etc. but I currently am in the situation that I don't want it globally. Think one route working like GitHubs source browsing.

I'm fine with putting turbo=true on all of them but currently it just gets ignored because Turbo doesn't deem it visitable despite me explicitly telling it that it is. Or since I guess it defaults to true maybe a data-turbo-force=true or something so that I can selectively tell Turbo to "trust me on this one".

[f6p]

@2called-chaos It doesn't. I think that this is a separate bug/issue. Right now its value could be true or false but I think it should accept 3 values false - always disable Turbo, default - run default check (what true does right now) and true - always force Turbo visit (not what it does currently). It probably would be what one would expect from it without studying internals.

I have similar project where I have to pass file path as a parameter in a host/controller/path/to/some/file.txt format and what I do is I modify is isHTML to always return true and manually disable likns that aren't navigable with data-trubo=false.

Cheers
🍷

[2called-chaos]

I think it should accept 3 values

Yeah I think the issue is that you can have Turbo Drive default to off and selectively enable it. That would mean data-turbo=true would be ambiguous if used as force toggle. I reckon a data-turbo-force would be easy though

willFollowLinkToLocation(link: Element, location: URL) {
  return this.elementDriveEnabled(link)
    && locationIsVisitable(location, this.snapshot.rootLocation)
    && this.applicationAllowsFollowingLinkToLocation(link, location)
}

would turn to something like this and I would be happy

willFollowLinkToLocation(link: Element, location: URL) {
  return this.elementDriveEnabled(link)
    && (
      link.getAttribute("data-turbo-force") == "true" ||
      locationIsVisitable(location, this.snapshot.rootLocation)
    )
    && this.applicationAllowsFollowingLinkToLocation(link, location)
}

Also how do you overwrite isHTML? I'm too dumb when it comes to es6 module scopes apparently

[f6p]

Yes, I don't think it will be often used but true and false don't cover all use cases.

As for overwriting isHTML its not so fancy 😃 I just have a local branch with isHTML implementation I need and I build Turbo from source using it.

[dahlbyk]

Would love to see #519 land!

Yeah I think the issue is that you can have Turbo Drive default to off and selectively enable it. That would mean data-turbo=true would be ambiguous if used as force toggle.

I tentatively disagree with this. Selectively enabling when Drive is off is telling Turbo to handle the link/form/container. I can't imagine why you'd expect Turbo to end up not handling where you opted in for any reason, let alone because of a . in the URL. And you can always data-turbo="false" where it causes issues.

[zarqman]

Just ran into this same issue when using an FQDN as a param at the end of a path (in a Rails app), eg: <a href="/validate/example.com" data-turbo-method="post">. Since the link already included data-turbo-method="post", I was particularly confused why Turbo was ignoring the link and it turning into a GET request.

Yeah, we need to be backward compatible, so we can't change the default behavior.

Since a new major version, aka Turbo 8, is coming up, can this logic now be changed in a breaking way?

I'd personally prefer dropping isHTML(), assuming all links are safe, and relying on adding data-turbo=false to bypass.

However, if that's still too much even for a major release, perhaps the logic could be inverted to become default-allow unless an extension is on the known-unsafe list (containing .jpg, .mp4, etc.)?

[DaAwesomeP]

Just a note that Rails will often append /my_path.id (i.e. /my_path.23) to a resource if accessing a specific resource. Per this issue this form will be ignored. From #1069.

[brunoprietog]

You can add a / to the end of the path and Turbo will handle the visits correctly. /users/jason.json/

In Rails, you can use trailing_slash option:

resources :users, trailing_slash: true

You can see more at ActionDispatch::Routing::UrlFor documentation

[edwinv]

We are using a specific URL syntax for describing date periodes, containing dots. We suddenly had Turbo do full page refreshes instead of morphing the page for streaming page refreshes. It took quite some debugging to find this issue. I think isHTML is a code smell, because it can't determine the extension of the path correctly in all situations. An URL with a dot is still valid, but Turbo just uses the last part as an extension and tries to determine if it is a HTML request. It is extra confusing because Turbo is just doing a page refresh, so it could already know that the current page it is refreshing is loadable as HTML.

Reading all the comments above, I would like to suggest another possible approach:

1. The isHTML check is removed and replaced with an isKnownNonHtmlPath. This method returns true when well known extensions like png are included in the path. In that case it is safe to assume we don't want to load with Turbo.
2. For all other cases, we proceed with the visit. When the request is received, we check the returned mime type to double check if we got HTML.
3. When a non-HTML response is returned, we make a new request with location.href = '...' to load the request. The second request is a tradeoff for URLs that don't have a known non-HTML extension and links that don't have data-turbo="false" added, but it makes sure the URL is still being loaded. Users have two options to prevent the double request. We could log a message to the console to suggest the user to make improvements to prevent double requests.

I think this approach would be backwards compatible with the current situation and solves most edge cases with the current isHTML approach.

[chillenberger]

I ran into this issue while making a content management system. It was important to us that the url matched the file title, and some of the titles are function calls that contain a ".".

My solution was a data attribute on the calling link that stated the href isVisitable. I pass the link element to the locationIsVisitable function, check for the data attribute, and return true if it exists, else I use the original check of isHTML and isPrfixedBy. I am not super familiar with the code base so this might not be a great solution. If anyone is interested I can submit a PR.

I am pretty sure this is backwards compatible, and is nice if you only have a couple urls on your site that have this issue.

[edwinv]

@chillenberger in our case that would mean we need to add the isVisitable state to many links. And beforehand we don't know which links that state need. Our URLs contain a dynamic part that might contain a range with .. in it. We use this construction on many index pages and are linking to these pages from many locations. Having to mark each link to be visitable would mean a lot of work.

I think linking to non-HTML pages is a lot less common overall. In our application we only link to things like PDF, CSV and Excel exports. Having to mark these links with data-turbo="false" would be perfectly fine because they are focused on a specific action instead of in the regular navigation flow of the application.