rubyforgood / casa

Volunteer management system for nonprofit CASA, which serves foster youth in counties across America.
https://casavolunteertracking.org/
MIT License
301 stars 467 forks source link

add turbo-rails #5906

Open elasticspoon opened 1 month ago

elasticspoon commented 1 month ago

What type(s) of user does this feature affect? Steve Ballmer DEVELOPERS DEVELOPERS DEVELOPERS

Description

We would like to have more SPA (Single Page Application) esque behavior in parts of the application. The common way to do this in Rails is via Hotwire https://hotwired.dev/.

We currently already have Stimulus so lets add Turbo.

Criteria for Completion

QA Login Details:
Link to QA site

Login Emails:

password for all users: 123456

Questions? Join Slack!

We highly recommend that you join us in slack https://rubyforgood.herokuapp.com/ #casa channel to ask questions quickly and hear about office hours (currently Wednesday 6-8pm Pacific), stakeholder news, and upcoming new issues.

elasticspoon commented 1 month ago

@mononoken I wrote this up for you :heart:

Feel free to comment and take it 🙂

mononoken commented 1 month ago

@elasticspoon thank you! I will take this one 🙂

mononoken commented 1 month ago

@elasticspoon , I started by just installing turbo-rails along with redis. This breaks about 20 tests. Why? Turbo includes Turbo Drive which is now intercepting all link and form requests by default.

It may be nice to replace the ujs implementations with Turbo Drive. I know the Rails guide recommends doing so (https://guides.rubyonrails.org/working_with_javascript_in_rails.html#replacements-for-rails-ujs-functionality). However, I am not sure if that was intended for this PR, since we had discussed only Turbo Frames previously. Plus, it was specified " tiny bit of turbo" and this technically applies it everywhere 😅.

I can add the config to disable Turbo Drive by default so that it is opted out by default but can be opted in. We could save the adoption of Turbo Drive as default for later, piecemeal or entirely. And then this PR can just focus on applying Turbo frames.

mononoken commented 1 month ago

I think the main things that have to be done to migrate to Turbo Drive:

  1. Update render calls for form actions in controllers to status 422 where applicable.
  2. Update JavaScript to use Stimulus everywhere. It looks like because Turbo Drive no longer performs full page reloads, some stuff written with jQuery is not getting loaded or attached when navigating between links anymore. I think these should be solved by using Stimulus instead, but I am not positive.
  3. Update ujs implementation in app/javascript/sweet-alert-confirm.js.
  4. Add data: {turbo: false} anywhere we want to keep full page reloads.

I noticed a side quest as well:

  1. There are a lot of links with methods defined other than get in the app using ujs for the AJAX request. From my understanding, this used to be common but is now frowned upon by some (see: https://www.youtube.com/watch?v=k7iT2cL-J0w). It is preferred to use button_to to generate a form instead.

I am down to work to get all of the above done, but I think a piecemeal approach would be best for review purposes. And I am not sure how much of a priority would be for these changes which I think are mostly potential performance improvements. If y'all think it's best to get it all done in one though, I am happy to do that too.

How should I proceed?

elasticspoon commented 1 month ago

@elasticspoon , I started by just installing turbo-rails along with redis. This breaks about 20 tests. Why? Turbo includes Turbo Drive which is now intercepting all link and form requests by default.

Is the redis bit mandatory? I would prefer to avoid having to figure out how to make that work in production. At least for now.

  1. Update JavaScript to use Stimulus everywhere. It looks like because Turbo Drive no longer performs full page reloads, some stuff written with jQuery is not getting loaded or attached when navigating between links anymore. I think these should be solved by using Stimulus instead, but I am not positive.

Is it that we need to migrate to stimulus or do we just need to make sure that the JavaScript makes it into application.js that is loaded in the header?

I think the main things that have to be done to migrate to Turbo Drive:

  1. Update render calls for form actions in controllers to status 422 where applicable.

  2. Update ujs implementation in app/javascript/sweet-alert-confirm.js.

  3. Add data: {turbo: false} anywhere we want to keep full page reloads.

  4. There are a lot of links with methods defined other than get in the app using ujs for the AJAX request. From my understanding, this used to be common but is now frowned upon by some (see: https://www.youtube.com/watch?v=k7iT2cL-J0w). It is preferred to use button_to to generate a form instead.

I feel like every point you have brought up seems about PR sized. That said I think the only thing I might change is I think it might make sense do the PRs that prep for Turbo first then actually add it. So that would be like:

mononoken commented 1 month ago

@elasticspoon , I started by just installing turbo-rails along with redis. This breaks about 20 tests. Why? Turbo includes Turbo Drive which is now intercepting all link and form requests by default.

Is the redis bit mandatory? I would prefer to avoid having to figure out how to make that work in production. At least for now.

No, Redis is not mandatory. It's only necessary for some Turbo Stream features that we don't need to use. I can remove Redis.

  1. Update JavaScript to use Stimulus everywhere. It looks like because Turbo Drive no longer performs full page reloads, some stuff written with jQuery is not getting loaded or attached when navigating between links anymore. I think these should be solved by using Stimulus instead, but I am not positive.

Is it that we need to migrate to stimulus or do we just need to make sure that the JavaScript makes it into application.js that is loaded in the header?

You are right, that we could also edit the JavaScript to make sure it gets loaded. For example, using delegation fixes attachment of this event listener:

// app/javascript/src/casa_case.js:133
  $('button.copy-court-button').on('click', copyOrdersFromCaseWithConfirmation) // old
  $(document).on("click", "button.copy-court-button", copyOrdersFromCaseWithConfirmation); // new

This would be a quicker fix.

I think the main things that have to be done to migrate to Turbo Drive:

  1. Update render calls for form actions in controllers to status 422 where applicable.
  2. Update ujs implementation in app/javascript/sweet-alert-confirm.js.
  3. Add data: {turbo: false} anywhere we want to keep full page reloads.
  4. There are a lot of links with methods defined other than get in the app using ujs for the AJAX request. From my understanding, this used to be common but is now frowned upon by some (see: https://www.youtube.com/watch?v=k7iT2cL-J0w). It is preferred to use button_to to generate a form instead.

I feel like every point you have brought up seems about PR sized. That said I think the only thing I might change is I think it might make sense do the PRs that prep for Turbo first then actually add it. So that would be like:

  • change render calls
  • fix sweet alert
  • deal with jquery > stimlus
  • add turbo with it being disabled and opt in
  • slowly enable it in places

Yeah, my main concern was a big PR being potentially annoying to review, and that it would be unexpected. If that is not an issue, I don't mind just doing them all in one. 😁 I think your order sounds best, esp if we split the work up.

elasticspoon commented 1 month ago

No, Redis is not mandatory. It's only necessary for some Turbo Stream features that we don't need to use. I can remove Redis.

Cool lets not bring that in because I don't think there is a strong use case for websocket stuff atm and we do jobs via DB atm.

You are right, that we could also edit the JavaScript to make sure it gets loaded.

This would be a quicker fix.

Cool. I think we should do the minimum changes.

Yeah, my main concern was a big PR being potentially annoying to review, and that it would be unexpected. If that is not an issue, I don't mind just doing them all in one. 😁 I think your order sounds best, esp if we split the work up.

I would very much appreciate multiple small PRs. But feel free to stack them. As in when you replace render stuff open a new PR that relies on that one with JS changes, etc.

github-actions[bot] commented 1 month ago

This issue has been inactive for 241 hours (10.04 days) and will be unassigned after 119 more hours (4.96 days). If you have questions, please

If you are still working on this, comment here to tell the bot to give you more time

mononoken commented 1 month ago

Still working on this one. I have the first PR submitted, but my schedule got busy this week so may take some time getting the other parts done. I intend to finish it but let me know if there are any issues with this going too slow.

elasticspoon commented 1 month ago

Still working on this one. I have the first PR submitted, but my schedule got busy this week so may take some time getting the other parts done. I intend to finish it but let me know if there are any issues with this going too slow.

Feel free to tell the bot to shut up. I have also been super busy so its no rush.

mononoken commented 4 weeks ago

@elasticspoon btw this got closed but #5930 only completed part of this one. Think this should still be open unless we separate the other tasks into separate issues.

github-actions[bot] commented 2 weeks ago

This issue has been inactive for 240 hours (10.00 days) and will be unassigned after 120 more hours (5.00 days). If you have questions, please

If you are still working on this, comment here to tell the bot to give you more time

mononoken commented 2 weeks ago

This issue has been inactive for 240 hours (10.00 days) and will be unassigned after 120 more hours (5.00 days). If you have questions, please

If you are still working on this, comment here to tell the bot to give you more time

Back from my trip and looking at this again.

github-actions[bot] commented 4 days ago

This issue has been inactive for 240 hours (10.00 days) and will be unassigned after 120 more hours (5.00 days). If you have questions, please

If you are still working on this, comment here to tell the bot to give you more time