luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.57k stars 156 forks source link

Turbolinks legacy module #1650

Closed rmarronnier closed 2 years ago

rmarronnier commented 2 years ago

Follow-up / Replace https://github.com/luckyframework/lucky/pull/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

rmarronnier commented 2 years ago

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

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.