Turbolinks legacy module

[rmarronnier]

Follow-up / Replace #1648
Following various discussions about the fate of Turbolinks support in Lucky, this PR extract Turbolinks specific code that was hardcoded in redirectable.cr to a single module file turbolinks_action_support.cr.

This module can be included into a Lucky::Action to regain Turbolinks support.

This is a necessary step for Lucky to be able to support alternatives (like Turbo Drive for example) without monkeypatching redirectable.cr

Right now, in the current state of this PR, an existing Lucky project needs to include Lucky::TurboLinksActionSupport into every related action or into an "umbrella" action to keep Turbolinks support like this spec example :

class RedirectTurboLinksAction < TestAction
  include Lucky::TurboLinksActionSupport

  get "/redirect_turbolinks_test" do
    plain_text "does not matter"
  end
end

I'm open to more elegant / efficient refactoring / naming / ... changes.

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]

This is looking good! Really glad to see that you were able to move all the turbolinks stuff into the same module. My concern is that I just know people will miss that they need to include the module. I think I'd prefer that we still include the turbolinks module for this next release. The new turbo module, though, would just override the special_ajax_redirect method (or whatever it might be renamed to) and have an empty method for set_turbolinks_location_header_from_session so that if the turbo module and turbolinks module are present, the turbo one takes precedent. And I think it would probably be best to switch to including the turbo module in generated projects (so in LuckyCli) instead of the Lucky::Action so that this situation can be solved outside of updating lucky directly in the future. Make sense?

[rmarronnier]

This is looking good! Really glad to see that you were able to move all the turbolinks stuff into the same module. My concern is that I just know people will miss that they need to include the module. I think I'd prefer that we still include the turbolinks module for this next release. The new turbo module, though, would just override the special_ajax_redirect method (or whatever it might be renamed to) and have an empty method for set_turbolinks_location_header_from_session so that if the turbo module and turbolinks module are present, the turbo one takes precedent. And I think it would probably be best to switch to including the turbo module in generated projects (so in LuckyCli) instead of the Lucky::Action so that this situation can be solved outside of updating lucky directly in the future. Make sense?

It makes perfect sense : the "double-overriding" was the missing piece of the puzzle for a non-breaking change path. Thank you :-)

[rmarronnier]

I pushed the discussed changes and I renamed the module back to Lucky::RedirectableTurbolinksSupport, which is more accurate (doesn't change behavior outside of redirectable.cr).

Ready for (final ?) review.

[matthewmcgarvey]

This PR got tiny! Nice work 🎉