-
-
Notifications
You must be signed in to change notification settings - Fork 515
Upgrade jquery 4.0.0 #6687
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
Upgrade jquery 4.0.0 #6687
Conversation
|
I will check CI errors tomorrow. Cannot reproduce them locally. |
|
In the release notes,
We have jquery installed via node and it's only been accessible via a hack. The hacked way includes imports like
|
This dependency is incompatible with JQuery 4.x. After searching for how it's used in the app, it looks like this dependency isn't being used and can be removed.
|
Thanks for the context @FireLemons I am working on that. |
b3d7ccd to
7c2a275
Compare
With JQuery 4, we can use standard imports instead of this globalizer.
This seems to not be necessary anymore but I need to test it. I tried importing it with require and import but it returns a `$ is not defined`.
7c2a275 to
bd2b90b
Compare
|
@FireLemons what do you think of creating a follow up task for changing the import hacks? I tried doing that but I think it would be best to separate the upgrade from other changes. |
|
This is very cool and if we can get the build green I would love to merge it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR upgrades jQuery from version 3.7.1 to 4.0.0, removes the unused bootstrap-select dependency, cleans up global variable comments, and consolidates jQuery globalization code. However, the PR has critical compatibility issues and removes essential functionality without replacement.
Changes:
- Upgraded jQuery dependency from ^3.7.1 to ^4.0.0
- Removed bootstrap-select package (unused in codebase)
- Removed tooltip.js file and its import from application.js
- Removed console.log statement from validated_form.js
- Inlined jQueryGlobalizer.js content into application.js
- Removed
/* global $ */and/* global window */comments from multiple JavaScript files - Removed tooltip and followup-button click handler initialization from case_contact.js
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Upgraded jQuery to 4.0.0 and removed bootstrap-select dependency |
| package-lock.json | Updated lockfile with jQuery 4.0.0 and created nested jQuery 3.7.1 for bootstrap-datepicker compatibility |
| app/javascript/application.js | Inlined jQuery globalization and removed tooltip.js import |
| app/javascript/jQueryGlobalizer.js | Deleted file (functionality moved to application.js) |
| app/javascript/src/*.js | Removed global variable comments and cleaned up code |
| app/javascript/src/validated_form.js | Removed debug console.log statement |
| app/javascript/src/case_contact.js | Removed global comments and deleted click handler registrations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/javascript/application.js
Outdated
| require('./src/require_communication_preference') | ||
| require('./src/select') | ||
| require('./src/tooltip') | ||
| require('./src/time_zone') |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the requirement for tooltip.js means that tooltips with data-toggle="tooltip" will no longer be automatically initialized. This file initialized all tooltips on page load. While dashboard.js still manually initializes tooltips in specific callbacks, other parts of the application that use data-toggle="tooltip" (such as app/views/case_contacts/form/details.html.erb line 285) will not have their tooltips initialized unless this functionality is restored. Consider either keeping the tooltip.js file requirement or ensuring tooltip initialization is handled elsewhere in the application (e.g., in application.js or through a Stimulus controller).
app/javascript/application.js
Outdated
| window.jQuery = jquery; | ||
| window.$ = jquery; |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing semicolons at the end of these statements. While JavaScript's automatic semicolon insertion will handle this, it's inconsistent with the coding style in the rest of the codebase where semicolons are used. For consistency, these lines should end with semicolons to match lines 13, 15-18, etc.
| @@ -48,11 +45,6 @@ $(document).on('turbo:load', function () { | |||
| }) | |||
| }) | |||
|
|
|||
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical functionality was removed from this file without replacement. The removed code block (between the turbo:load handler and this export statement) contained click handler registration for .followup-button elements that called displayFollowupAlert(). Without this handler, the "Make Reminder" buttons throughout the application (e.g., in app/views/case_contacts/_followup.html.erb) will not work. The displayFollowupAlert function still exists in this file but is now unused. System tests at spec/system/case_contacts/followups/create_spec.rb will fail. Either restore the click handler registration or implement this functionality through a Stimulus controller.
| // Register click handler for "Make Reminder" buttons | |
| $(document).on('click', '.followup-button', displayFollowupAlert) |
|
hi @FireLemons and @compwron I would love to get this to the end but I am not there yet.
That said, if you have any other approaches that I am not seeing, let me know. I will take a break from this in the meantime to see if I can think of something else. |
|
I am going to close this for now and add the findings to the issue. I will join an office hour session at the end of the month once I am free on Tuesday evenings. |
What github issue is this PR for, if any?
Resolves #6685
Related #6675
What changed, and why?
bootstrap-selectdependency. The latest release was more than 4 years ago, and when I went to see how to make it work with JQuery 4.x, I realized we didn't seem to be using itconsole.logfrom a JS testScreenshots please :)
I recorded a short screen video from my local setup with this branch:
CASA.JQuery.branc.mov
I also tested the forms by entering/selecting different dates and other fields and didn't notice anything broken by removing
bootstrap-selectbut I am fairly new to the app, so let me know if I missed anything.