-
Notifications
You must be signed in to change notification settings - Fork 8
Update jQuery from 1.12.4 to 3.6.3 #1280
Conversation
Although there are many reasons to motivate an upgrade of such a legacy dependency, the main motivation for this change was that jQuery 1.12 triggers CSP violations on initialisation in Firefox (due to [1] which was removed by jQuery 2.2). The reason this uses such an old version of jQuery is that we (GOV.UK) used to share a version of jQuery across many apps and thus it was scary to update as it was hard to test it's impacts. Since March 2022 [2] Licence Finder has had it's own version of jQuery and thus it is easier to judge the impacts. To determine compatibly with jQuery 3 I ran this through jQuery migrate [3] and fixed the one warning. I also went through and manually tested the code setting breakpoints to check various parts were executed. [1]: https://github.com/jquery/jquery/blob/e09907ce152fb6ef7537a3733b1d65ead8ee6303/src/event/support.js#L7-L20 [2]: c384fc5 [3]: https://github.com/jquery/jquery-migrate
This is so that dependabot can raise PRs for changes in dependencies referenced in the package.json file, which is currently just jQuery. My motivation for adding this is noticing we weren't getting dependabot PRs to update jQuery from 1.12.
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.
Changes look good to me 👍
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.
Licence Finder still works in Firefox and Chrome in integration, and I couldn't see any CSP violations in the Firefox console, so 👍
@@ -13,7 +13,7 @@ $(function() { | |||
|
|||
// $('el:bottom-offscreen') will return true if the element's | |||
// bottom box border is off the screen | |||
$.expr.filters['bottom-offscreen'] = function(el) { | |||
$.expr.pseudos['bottom-offscreen'] = function(el) { |
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.
Why has this method changed? Was filters
replaced?
Ah, I see that it was: https://github.com/jquery/jquery-migrate/blob/69a24416078538ca267c6182207611002cdb9908/src/jquery/core.js#L102-L103
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.
Yup, deprecated in jQuery 3.
Thanks for the review Leena ⭐ 🙇
Trello: https://trello.com/c/lxxx5XLZ/178-govuk-has-a-half-implemented-content-security-policy-csp
Although there are many reasons to motivate an upgrade of such a legacy
dependency, the main motivation for this change was that jQuery 1.12
triggers CSP violations on initialisation in Firefox (due to 1 which
was removed by jQuery 2.2).
The reason this uses such an old version of jQuery is that we (GOV.UK)
used to share a version of jQuery across many apps and thus it was scary
to update as it was hard to test it's impacts. Since March 2022 2
Licence Finder has had it's own version of jQuery and thus it is easier
to judge the impacts.
To determine compatibly with jQuery 3 I ran this through jQuery migrate
3 and fixed the one warning. I also went through and manually tested
the code setting breakpoints to check various parts were executed.