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

Links which specify a data-method have that method ignored #33

Closed
bhaibel opened this issue Dec 24, 2020 · 5 comments
Closed

Links which specify a data-method have that method ignored #33

bhaibel opened this issue Dec 24, 2020 · 5 comments

Comments

@bhaibel
Copy link

bhaibel commented Dec 24, 2020

Let's say I have the following in a template:

<%= link_to "Sign out", signout_path, method: :delete %>

When I click on it, I'll get a routing error that seems to reflect the link having been visited with a GET rather than a DELETE request. I believe that this means that turbo-rails is not picking up on the data-method in the rendered link.

@dhh
Copy link
Member

dhh commented Dec 25, 2020

I thought we had already pulled this in from UJS, but guess not. @sstephenson thoughts on how to get this in?

Although an alternative would be to say that turning ahrefs into POSTs is a bit of an anti-pattern, especially for a11y reasons. Better to use button_to with a styling. I think that's why this didn't come up in HEY, since we've been removing links that mutate in part for a11y reasons.

But if that approach is followed, we clearly need to document it.

@dhh
Copy link
Member

dhh commented Dec 25, 2020

@seanpdoyle Any thoughts on the a11y aspect?

@bhaibel
Copy link
Author

bhaibel commented Dec 25, 2020

The a11y point is a really good one.

I still think we should port the old rails-ujs behavior in, at least for now (maybe it gets a deprecation warning slapped on it?) -- my instinct is that data-method links suddenly Stopping Working is going to be a point of confusion & frustration for folks trying to transition from UJS to Turbo.

From a developer UX perspective, my long-term ideal might be something that warns the developer they are doing a stupid a11y thing, but that might not be worth the extra kb over the wire.

@seanpdoyle
Copy link
Contributor

seanpdoyle commented Dec 25, 2020

I agree, combining link_to and destructive HTTP Verbs is troublesome. From my perspective, the underlying mismatch between the semantics of a destructive action like an HTTP DELETE and an <a> element complicate the matter.

While the accessibility issues can be papered-over by declaring role="link", it might still surprise users of assistive tech if clicking a link triggered anything but idempotent behavior.

From a developer UX perspective, my long-term ideal might be something that warns the developer they are doing a stupid a11y thing, but that might not be worth the extra kb over the wire.

I've opened rails/rails#40935 against rails/rails, and consider it a temporary bridge toward a long-term improvement. It doesn't warn the developer directly, but it tries to remedy the underlying issue. It shifts the HTTP verb work-around from JavaScript (either RailsUJS or a Turbo polyfill of RailsUJS which may or may not be present or enabled) to the server side via the tried-and-true _method=VERB hidden input element.

I still think we should port the old rails-ujs behavior in, at least for now (maybe it gets a deprecation warning slapped on it?) -- my instinct is that data-method links suddenly Stopping Working is going to be a point of confusion & frustration for folks trying to transition from UJS to Turbo.

I was reflecting on the same issue. I wonder if there is a bridge/polyfill of sorts that we could provide as part of turbo-rails that would map Turbo events to UJS events. I think there's close overlap between:

  • turbo:before-visit and ajax:before
  • turbo:before-fetch-request and ajax:beforeSend
  • turbo:submit-start and ajax:send
  • turbo:before-fetch-response and ajax:success + ajax:error + ajax:complete (depending on the HTTP status in the response)

There isn't a total overlap, but I wonder if there is enough to provide our own Form Submission lifecycle listeners (plus 2 others), while still re-using the majority of Rails.start(). I know that Turbo.start() is invoked during the import call, but maybe we can supplement applications with another module they can import and start as a polyfill of sorts.

I've opened hotwired/turbo#40 to test this out. Its focus was mostly on [data-disable-with], but would complement the changes proposed in rails/rails#40935.

@dhh
Copy link
Member

dhh commented Dec 30, 2020

Documented the UJS compatibility and stopped removing it from a Webpack app by default.

@dhh dhh closed this as completed Dec 30, 2020
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Jan 25, 2021
Closes hotwired#33.
Closes hotwired#37.

---

Re-create [hotwired/turbo#40][].

When loaded alongside [@rails/ujs][], start it via `Rails.start()`, and
then declare the [`turbo:`-prefixed events][turbo-events] that
correspond to their `ajax:`-prefixed counterparts.

Test suite changes
---

First, introduce System Test coverage to drive the Turbo and Rails
integration through a headless browser.

Next, change some dummy controller actions to accept a `?sleep=` query
parameter to delay their responses long enough to ensure that
`data-disabled-with` and `data-confirm` have a wide enough window of
opportunity for test coverage to exercise their behaviors.

[hotwired/turbo#40]: hotwired/turbo#40
[@rails/ujs]: https://github.com/rails/rails/tree/v6.1.0/actionview/app/assets/javascripts
[turbo-events]: https://turbo.hotwire.dev/reference/events
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Jan 25, 2021
Closes hotwired#33.
Closes hotwired#37.

---

Re-create [hotwired/turbo#40][].

When loaded alongside [@rails/ujs][], start it via `Rails.start()`, and
then declare the [`turbo:`-prefixed events][turbo-events] that
correspond to their `ajax:`-prefixed counterparts.

Test suite changes
---

First, introduce System Test coverage to drive the Turbo and Rails
integration through a headless browser.

Next, change some dummy controller actions to accept a `?sleep=` query
parameter to delay their responses long enough to ensure that
`data-disabled-with` and `data-confirm` have a wide enough window of
opportunity for test coverage to exercise their behaviors.

[hotwired/turbo#40]: hotwired/turbo#40
[@rails/ujs]: https://github.com/rails/rails/tree/v6.1.0/actionview/app/assets/javascripts
[turbo-events]: https://turbo.hotwire.dev/reference/events
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Jan 25, 2021
Closes hotwired#33.
Closes hotwired#37.

---

Re-create [hotwired/turbo#40][].

When loaded alongside [@rails/ujs][], start it via `Rails.start()`, and
then declare the [`turbo:`-prefixed events][turbo-events] that
correspond to their `ajax:`-prefixed counterparts.

Test suite changes
---

First, introduce System Test coverage to drive the Turbo and Rails
integration through a headless browser.

Next, change some dummy controller actions to accept a `?sleep=` query
parameter to delay their responses long enough to ensure that
`data-disabled-with` and `data-confirm` have a wide enough window of
opportunity for test coverage to exercise their behaviors.

[hotwired/turbo#40]: hotwired/turbo#40
[@rails/ujs]: https://github.com/rails/rails/tree/v6.1.0/actionview/app/assets/javascripts
[turbo-events]: https://turbo.hotwire.dev/reference/events
seanpdoyle added a commit to seanpdoyle/turbo-rails that referenced this issue Jan 25, 2021
Closes hotwired#33.
Closes hotwired#37.

---

Re-create [hotwired/turbo#40][].

When loaded alongside [@rails/ujs][], start it via `Rails.start()`, and
then declare the [`turbo:`-prefixed events][turbo-events] that
correspond to their `ajax:`-prefixed counterparts.

Test suite changes
---

First, introduce System Test coverage to drive the Turbo and Rails
integration through a headless browser.

Next, change some dummy controller actions to accept a `?sleep=` query
parameter to delay their responses long enough to ensure that
`data-disabled-with` and `data-confirm` have a wide enough window of
opportunity for test coverage to exercise their behaviors.

[hotwired/turbo#40]: hotwired/turbo#40
[@rails/ujs]: https://github.com/rails/rails/tree/v6.1.0/actionview/app/assets/javascripts
[turbo-events]: https://turbo.hotwire.dev/reference/events
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants