Skip to content
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

Override isHTML or locationIsVisitable? #185

Closed
warpr opened this issue Feb 22, 2021 · 9 comments
Closed

Override isHTML or locationIsVisitable? #185

warpr opened this issue Feb 22, 2021 · 9 comments

Comments

@warpr
Copy link

warpr commented Feb 22, 2021

Hello!

With turbolinks I used to do this:

Turbolinks.Location.prototype.isHTML = () => true;

Is this no longer possible with Turbo?

The default extension regex often trips me up, as a PHP developer I obviously expect <turbo-frame id="messages" src="/messages.php">msg</turbo-frame> to work.

In my case some of my URLs happen to have periods in them, which Turbo fails to load correctly (e.g. /music/prince/1989.batman) because it parses the ".batman" part as the extension.

I also tried setting data-turbo=true, as setting data-turbo=false forces links to not be navigable or visitable, so I figured maybe data-turbo=true forces it in the other direction, but it seems to just give me the default behavior.

For now, the workaround I'm using is making sure my URLs end in a slash, as that seems to bypass the extension check, but it would be nice if there was some way to force Turbo to treat a particular link as navigable/visitable.

@mbtuckersimm
Copy link

Just chiming in to say that this is an issue for my team also. I saw the PR that removed the Location class (#90), and that seems like a solid engineering decision, but now that it's not possible to patch Location.isHTML, we're not able to upgrade. The comment here is still applicable:

Judging from the above, it looks like 5.3 will break PHP apps using the common https://domain.com/endpoint.php pattern, with routing handled directly by Apache (yes, we’re still here). We have a few not-great choices:

  • freeze at 5.2
  • fork
  • major refactor

I think our team will probably freeze at 5.2 until we can refactor (it’s not like we’re thrilled with this kind of routing) but I wanted to leave this comment as evidence that the problem is not an edge case for older php apps and therefore might be worth a little bump in priority.

We've already upgraded to Stimulus 2.0 and we'd love to upgrade to Turbo as well. It seems a shame that there's no way to override the set of extensions that Turbo deems eligible for visiting. I really hope you'll consider making this possible again (in a better way than monkey-patching).

@ajlive
Copy link

ajlive commented Mar 4, 2021

This would be a big boon to small teams that don't have the resources to refactor all their routing just to support turbo. Some of us are in a very dire situation with our legacy code in this area ;).

My team would be happy to submit a PR implementing this if the turbo team feels like this is worth supporting and if a design is settled on.

@ajlive
Copy link

ajlive commented Jul 29, 2021

Bumping because we still have this issue and would very much like to see it resolved or even help resolve it.

We're currently monkey-patching turbo to accept .php urls.

@kasperkamperman
Copy link

kasperkamperman commented Jul 29, 2021

Indeed it would be great if Turbo would support other extensions like .php as well. I know it's now part of the Rails community, but I think it's a great and easy way to create a SPA experience with PHP generated websites as well in a very simple way (Laravel Livewire needs Laravel).

See also my discussion topic at:
https://discuss.hotwired.dev/t/how-to-make-turbo-drive-work-with-php-files-and-query-strings/2020

@ajlive
Copy link

ajlive commented Sep 24, 2021

I'd like to present an argument that this issue should be a little higher priority: Hotwire is being marketed as being about JS on the client and HTML on the server, not as an extension of Ruby on Rails, so it seems strange that it excludes many PHP sites, which last I checked still accounts for some 80% of server-side code on the web. Yes, not all of PHP sites out there use urls that end in .php, but I'm not sure why Turbo, as a client JS library, cares what my urls look like.

My team would be happy to work with maintainers to find a more acceptable way to configure than isHTML and make a PR. I'd love to hear any response from the maintainers about this, even if it's "wont fix" so that I know we need to fork turbo.

@seanpdoyle
Copy link
Contributor

Issues related to this have popped up several times since this issue was opened in February 2021, most recently in #491 (comment) due to a change introduced in 7.1.0 (from #437).

An internal or user-facing solution is definitely worth exploring.

@f6p f6p mentioned this issue Jan 24, 2022
@bteller
Copy link

bteller commented Jun 25, 2022

Lots of conversation here and wondering if I can't suggest what might be a simpler approach. Could we not allow overrides in this manner?

Turbo.overrides.isHTML = () => { return true; }

Then within the Turbo isHTML function itself you could:

if (Turbo.overrides && Turbo.overrides.isHTML) { 
  return Turbo.overrides.isHTML(url);
}

@bteller
Copy link

bteller commented Jun 29, 2022

Leaving my previous comment as a source of shame. It hadn't dawned on me that there was already this concept of "session" that accepted settings. Please consider this PR which makes use of the current convention.

@dhh
Copy link
Member

dhh commented Jul 15, 2022

Let's consolidate the conversation on #519.

@dhh dhh closed this as completed Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

7 participants