Migrate Turbolinks to Turbo #2998
Conversation
MrSerth
added
dependencies
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2998 +/- ##
=======================================
Coverage 70.05% 70.05%
=======================================
Files 215 215
Lines 6846 6846
=======================================
Hits 4796 4796
Misses 2050 2050 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
MrSerth
force-pushed
the
ss/turbo-migration
branch
from
8713b17 to
d647dab
Compare
|
This PR is huge, but is works. I tested it locally and it works well. I added my first comment about the sign out of the session controller. Now I am considering to just approve the PR and changing individual things with later PRs. This way we can get this merged once. @MrSerth Should we proceed like this? |
This is the first part of using Turbo for this code base. Some solutions, like the temporary events triggered, should be temporary and removed once we fully migrated from Sprockets to another JavaScript distribution (such as Shakapacker, importmaps, etc.)
These links work correctly with Turbo, so that we don't need to opt out here. Also, we can cache the /implement route normally, since the LTI handling has changed in 2024.
Turbo imposes a new requirement for form submissions: Whenever a form is submitted, a redirect is expected. When no redirect is performed (i.e., when the data submitted didn't pass validation and the same form is rendered again), a 4xx or 5xx error code is required. Therefore, this commit changes those occurrences to return a 422 error code. Without this change, an error would be logged by Turbo in the JavaScript console and the response (including the flash message) wouldn't be shown. See https://turbo.hotwired.dev/handbook/drive#redirecting-after-a-form-submission
Turbo requires us to use the HTTP status code 303 for successful form submissions after stateful server changes. See https://turbo.hotwired.dev/handbook/drive#redirecting-after-a-form-submission
For these actions, we use use a non-default `method` (other than `GET`). To indicate that within the DOM (i.e., for Screenreaders) but also enhance compatibility with Turbo, we switch to proper `button_to` forms here. As part of this migration, some buttons should still be shown inline (hence, we add the `form_class: 'd-inline-block'`) or with the same height as regular links (hence, we add `class: 'h-100'`). See https://github.com/heartcombo/devise/wiki/How-To:-Upgrade-to-Devise-4.9.0-%5BHotwire-Turbo-integration%5D#data-method-vs-data-turbo-method See https://gorails.com/episodes/link_to-vs-button_to-in-rails
After migrating `link_to` to `button_to` (in the previous commit), we need to adjust the styles for a uniform and correct appearance.
In some cases, the left sidebar was too high, causing an undesired scroll bar to be displayed. We increase the magic number to hide the scrollbar in more cases. This is not a proper solution, but enough to hide the scroll bars once again. We need to investigate further.
When the form is submitted but no redirect occurs (e.g., after a validation error), the page content is simply replaced. However, for this occasion Turbo misses to emit a new `turbo:load` event, which makes it more difficult to use custom JavaScript waiting on page initializations. Potentially, we could use `turbo:render` here to fix these occasions.
With Turbo enabled on forms with custom JavaScript / CSS packs and a successful change (no validation error), the final page would be rendered twice: The first rendering would occur after sending the form data using Turbo. This response is fetched and potentially displayed (first visit). The resulting DOM contains a change in the packs (with `'data-turbo-track': 'reload'`), causing a page reload (without Turbo). This is the second visit. Through this "double-visit", any flash message intended to be shown after the form submission is simply "lost". Therefore, we disable Turbo in these cases. This holds also true for signout actions, that can (potentially) originate from any page, including those with custom packs, and redirect to the root page (without any additional packs).
With this new method, Tooltips are correctly initialized and potential loading errors/race conditions are prevented.
Without this change, the new path might be visited before the login succeeded. This causes authorization issues, where another login screen was shown.
The Score system specs are very tricky and error-prone with the current setup. Mostly, this is due to the headless Chrome browser used on CI, which has a weird race condition preventing a regular use. Until said issue is resolved, we add additional expectations (to wait for the page to finish loading). See SeleniumHQ/selenium#15273
With this change, each editor instance is destroyed upon changing the page. This prevents memory leaks and thus optimizes the performance. The editor will be restored upon visiting the sites again.
Similar to the ACE editor, we unload the JsTree to prevent memory leaks. Also, we use this opportunity to reduce code duplication.
In general, we can run system specs with four browser combinations: 1. Chrome 2. Chrome Headless 3. Firefox 4. Firefox Headless Unfortunately, Selenium has an ongoing issue with Chrome Headless and Alerts, making the test execution slow or causing random failures. Non-headless Chrome works fine. To overcome these issues, we switch to Firefox for the time being in GitHub Actions. Non-headless browsers are not supported in GitHub Actions.
While loading the multilang support only on the respective pages sounds like a good idea, it complicates matters with the Turbo migration (since we would need to opt-out of some cases). Also, the exercise description is used in too many places, whereas the snippet is rather short and the performance impact to load it globally is rather minimal. Amends 7d4dd2c
MrSerth
force-pushed
the
ss/turbo-migration
branch
from
d647dab to
88b2010
Compare
Thanks for taking a look. I am glad you tested the changes locally and found them working; that's a huge win. Usually, I'm not in favor of merging something that's likely to be revised right away due to "pending" review comments. That said, I agree the PR already covers a lot, and keeping it up-to-date could become time-consuming for both of us. To make things easier, I've kept all changes in smaller commits -- grouping repetitive patterns and adding explanations where needed. I also took your comment about the Turbo cache on logout actions as a cue to perform some additional adjustments (e.g., HTTP status changes, converting With that in mind, do you have a sense of how many remarks you're planning to make, or how "severe" they might be? |
There was a problem hiding this comment.
With that in mind, do you have a sense of how many remarks you're planning to make, or how "severe" they might be?
In an ideal situation I would split this PR into multiple steps and release them over time. I would do this to see the impact of individual changes for a day or two.
For example, the link to the button and the status code changes could be released independently.
I think the theme changing and editor unloading should also work woth Trubolinks.
However, everything is reviewed and tested now. To move fast, I approved this for a single merge/release.
Alright, thank you. I agree that some changes could have been extracted, but am also fine to merge it now and make further adjustments when needed 👍. |
This is the first part of using Turbo for this code base. Some solutions, like the temporary events triggered, should be temporary and removed once we fully migrated from Sprockets to another JavaScript distribution (such as Shakapacker, importmaps, etc.). More details are available in the dedicated commit messages. Related to #2952 and #2953. While I have tested the changes extensively, handling of the JavaScript events is not that easy and might require additional refinements later (based on Sentry events or other observations).