Skip to content

refactor: Move Sprocket JS files to Webpack (Part 1) #3025

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 1, 2025

Conversation

arkirchner
Copy link
Contributor

@arkirchner arkirchner commented Jul 18, 2025

JavaScript for RfCs, Flashes and the color mode can be moved without
modifications.

Confirmed features:

  • Flash messages
  • RfC report, comment, and resolve flow.
  • Theme changeing.

Relates to #3021

@arkirchner arkirchner self-assigned this Jul 18, 2025
@arkirchner arkirchner force-pushed the ak/move_sprocket_assets_to_webpack_part_1 branch 4 times, most recently from 795e570 to ad943d2 Compare July 18, 2025 08:19
@arkirchner arkirchner marked this pull request as ready for review July 18, 2025 08:35
Copy link

codecov bot commented Jul 18, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.08%. Comparing base (97de3f3) to head (95f91ba).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3025   +/-   ##
=======================================
  Coverage   70.08%   70.08%           
=======================================
  Files         215      215           
  Lines        6850     6850           
=======================================
  Hits         4801     4801           
  Misses       2049     2049           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arkirchner arkirchner requested review from MrSerth and christophblessing and removed request for MrSerth July 22, 2025 07:21
@arkirchner arkirchner force-pushed the ak/move_sprocket_assets_to_webpack_part_1 branch from ad943d2 to 12f21d0 Compare July 22, 2025 12:30
Copy link
Member

@christophblessing christophblessing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you migrate these exact files first? Are they somehow connected or is it because you did not have to make any adjustments to them?

Copy link
Member

@MrSerth MrSerth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the "independent" files color_mode_picker.js and flash.js (originating from lib), why did you decide to mingle them within the sprocket_asset_import folder rather than finishing the migration exemplary for them?

@arkirchner
Copy link
Contributor Author

@christophblessing
I choose to start with files that I worked with recently. It is easy for me to test this feature because I am still familiar. I stopped adding more files because I thought the amount would be enough for a first release.

@arkirchner
Copy link
Contributor Author

@MrSerth @christophblessing
If we have a lib (library) I think of some code that is independent from the application. Since UI and application changes will influence the functionality of these files it is application code.

I didn't move the CSS at this point. It still is in the lib folder. I didn't want to integrate CSS changes here.

@MrSerth
Copy link
Member

MrSerth commented Jul 28, 2025

@MrSerth @christophblessing If we have a lib (library) I think of some code that is independent from the application. Since UI and application changes will influence the functionality of these files it is application code.

Thanks for your answer. I am not fully convinced of your argumentation yet. For example, when looking at the color_mode_picker.js: Why is it not independent of the application? It is mostly copied over from the Bootstrap documentation as acknowledged in the comment and only adapted to work with Turbo. Otherwise, it is unchanged and should work with other Bootstrap-enabled sites as wel.

I didn't move the CSS at this point. It still is in the lib folder. I didn't want to integrate CSS changes here.

Mh, I see. What is your long-term plan for CSS, then?

@arkirchner
Copy link
Contributor Author

arkirchner commented Jul 28, 2025

Otherwise, it is unchanged and should work with other Bootstrap-enabled sites as well.

Well the event listener is my concern here. It is very coupled to application behavior. There might be reasons to keep this file in some kind of lib folder; however, there is not really a reason to create a lib folder for one file.

Mh, I see. What is your long-term plan for CSS, then?

I am not sure at the moment.

@arkirchner
Copy link
Contributor Author

@MrSerth I want to move forward with this. Surely keeping this in the lib folder cannot be that important.

@MrSerth
Copy link
Member

MrSerth commented Jul 30, 2025

@MrSerth I want to move forward with this. Surely keeping this in the lib folder cannot be that important.

Thanks for confirming we are on the same page.

I didn't expect you'd be waiting for me here, currently. I thought there would be a response to the long-term migration strategy involving CSS, some progress with finishing the migration of the two former "lib" files, or some update on your planned research on the Webpack loading order or other details in this discussion of Part 2.

That being said, I don't mind much of the folder, despite thinking that mingling parts of these "not-so-close" files together with the main application logic isn't completely desirable.

JavaScript for RfCs, Flashes and the color mode can be moved without
modifications.

Relates to #3021
@arkirchner arkirchner force-pushed the ak/move_sprocket_assets_to_webpack_part_1 branch from ddb6a43 to 721f80e Compare July 30, 2025 11:50
@arkirchner
Copy link
Contributor Author

Thanks for the feedback.

I went ahead and moved the CSS files into the app directory. Work on the Webpack will happen in the next steps.

@arkirchner arkirchner marked this pull request as draft July 30, 2025 14:17
@arkirchner arkirchner marked this pull request as ready for review July 31, 2025 09:13
@arkirchner arkirchner merged commit c8710f2 into main Aug 1, 2025
15 checks passed
@arkirchner arkirchner deleted the ak/move_sprocket_assets_to_webpack_part_1 branch August 1, 2025 15:13
arkirchner added a commit that referenced this pull request Aug 6, 2025
The `tubo:load` event is faster than the migration event. The color changing requires no sprocket code and can be controlled without the migration event. This removes the flickering in dark mode when navigating the page.

This change is possible because the theme-changing logic was moved to Webpacker and loads before Sprockets. Please see #3025

Related to #3040
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants