WIP : Replacing Turbolinks with Turbo

rmarronnier commented 2 years ago

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 (https://github.com/luckyframework/lucky_cli/issues/629#issuecomment-1013188779)

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 (https://github.com/luckyframework/lucky_cli/issues/629#issuecomment-832211997) 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

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

matthewmcgarvey commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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

rmarronnier commented 2 years ago

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

matthewmcgarvey commented 2 years ago

@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 commented 2 years ago

@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.

luckyframework / lucky