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

WIP : Replacing Turbolinks with Turbo #1648

Closed
wants to merge 3 commits into from

Conversation

rmarronnier
Copy link
Contributor

@rmarronnier rmarronnier commented Jan 15, 2022

Purpose

There is some discussion about replacing Turbolinks with Turbo (https://github.com/hotwired/turbo) :

One proposed way to push this discussion forward is through a PR (luckyframework/lucky_cli#629 (comment))

My own take on this issue is first to start replacing Turbolinks capabilities by its Turbo counterpart, mainly Turbo Drive (https://turbo.hotwired.dev/handbook/drive) before any discussion/action about the remaining part of Turbo (Frames and Streams).

Description

This is a "candid" PR, it's mostly a repo-wide string replace of "turbo-links" to "turbo".

The good news is that the specs pass.

The bad news is also that the specs pass : this comment (luckyframework/lucky_cli#629 (comment)) hints that some deliberate action is required from the user to deal with forms (either setting a specific data_turbo: false or returning a 40x status code). No spec is currently testing this behaviour.

I can open a sister PR in https://github.com/luckyframework/lucky_cli if this one is greenlighted.

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

@matthewmcgarvey
Copy link
Member

@rmarronnier since this is a breaking change for anyone using turbolinks, is there a way we could support both for a release? I'm fine with the lucky_cli PR because it would only affect new apps

@rmarronnier
Copy link
Contributor Author

rmarronnier commented Jan 15, 2022

Mmm, we could try adding back Lucky::RedirectableTurboLinksSupport but this specific code :

if ajax? && request.method != "GET"
      context.response.headers.add "Location", path

      # do not enable form disabled elements for XHR redirects, see https://github.com/rails/rails/pull/31441
      context.response.headers.add "X-Xhr-Redirect", path

      Lucky::TextResponse.new(context,
        "text/javascript",
        %[Turbo.clearCache();\nTurbo.visit(#{path.to_json}, {"action": "replace"})],
        status: 200)
    else
      if request.headers["Turbo-Referrer"]?
        store_turbo_location_in_session(path)
      end
      # ordinary redirect
      context.response.headers.add "Location", path
      context.response.status_code = status
      Lucky::TextResponse.new(context, "", "")
    end

would need to be configurable to deal with both TurboLinks and Turbo.
We could keep Turbolinks support as default and enable the switch to Turbo support through a config variable (which would be set to true in new generated apps.

@rmarronnier rmarronnier mentioned this pull request Jan 17, 2022
5 tasks
@rmarronnier
Copy link
Contributor Author

After more thought, I've worked on a PR to un-hardcode existing TurboLinks support : #1650

@rmarronnier rmarronnier changed the title Replacing Turbolinks with Turbo WIP : Replacing Turbolinks with Turbo Jan 17, 2022
@rmarronnier rmarronnier marked this pull request as draft January 17, 2022 19:18
@rmarronnier
Copy link
Contributor Author

Discussion in #1650 showed a preference for adding Turbo support in lucky_cli for new projects.
I'll continue working on luckyframework/lucky_cli#726

@matthewmcgarvey
Copy link
Member

matthewmcgarvey commented Jan 19, 2022

@rmarronnier I think it's fine to add the module to this shard but then only include it in lucky_cli. Sorry for the confusion. Personally, I picture a future where the turbo stuff is in its own shard

@rmarronnier
Copy link
Contributor Author

@matthewmcgarvey Thanks for the clarification ! And a specific Turbo shard makeqs sense. Meanwhile, I need to come up with new Lucky Flow specs to make sure the existing Turbolinks/UJS behavior is dealt with the Turbo migration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants