Skip to content
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

DEP Update jQuery to 3.x #1375

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli GuySartorelli commented Oct 4, 2022

I've added some comments in this PR to help explain why some of the changes have been made.

A full CI run including behat for all modules which have a behat config has been run on kitchen sink. Note that running kitchen sink without this PR also has some failures (because cwp and other modules change defaults which change the test conditions) so compare these two to see if the failures are the same:

I've also run this against installer:

I have also confirmed that unclecheese/display-logic still works

Parent issue

@GuySartorelli
Copy link
Member Author

GuySartorelli commented Oct 6, 2022

Functionality from client/src/legacy/LeftAndMain.menu.js has been removed because it isn't used, and was requiring an entire dependency (for a one-liner) which I would otherwise have had to update.

The example in that file doesn't even work as advertised - it gives the following output:
image
As you can see the styling for this is all over the place. It's a holdover from CMS 3 which actually broke part-way through CMS 3 itself, and hasn't been fixed or used since.

If anyone does want nested menus, there's a separate module for that.

Note: That module relies on some of this child menu functionality and related styling, so I won't remove all of it - but I will remove the "flyout" parts, which are not used at all.

@GuySartorelli GuySartorelli force-pushed the pulls/1/jquery3-upgrade branch 3 times, most recently from 1d335aa to 427991f Compare October 18, 2022 03:28
Copy link
Member Author

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

The following deprecated jQuery functions were replaced with their modern counterparts throughout:

Though note that some of these are deprecated, but not yet removed - in those cases I have largely not replaced these in thirdparty scripts

// require('../../../thirdparty/jquery-ui-themes/smoothness/jquery-ui.css');
require('../../../thirdparty/jquery-entwine/dist/jquery.entwine-dist.js');
require('../../../thirdparty/jquery-ui/jquery-ui.js'); // there is related css in styles/bundle.scss
require('../../../thirdparty/jquery-entwine/jquery.entwine.js');
Copy link
Member Author

Choose a reason for hiding this comment

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

Moved because we only need this one file - all the rest is unnecessary cruft.

require('jquery-sizes/lib/jquery.sizes.js');
require('../../../thirdparty/jstree/jquery.jstree.js');
// require('../../../thirdparty/stree/themes/apple/style.css');
require('../../../thirdparty/jquery-hoverIntent/jquery.hoverIntent.js');
Copy link
Member Author

Choose a reason for hiding this comment

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

This dependency was only used in one place, and the place it was used was for legacy CMS 3 code (refer to #1375 (comment))

@@ -220,7 +220,6 @@
padding-top: 17px;
}

.child-flyout-indicator,
Copy link
Member Author

Choose a reason for hiding this comment

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

For all the flyout removals, refer to #1375 (comment)

Comment on lines -127 to +129
padding = activeTab.offset().top - containerSouth.offset().top;
const offset = activeTab.offset();
padding = offset ? (offset.top - containerSouth.offset().top) : 0;
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is required because offset() returns undefined for hidden elements.

While it is possible to get the coordinates of elements with visibility:hidden set, display:none is excluded from the rendering tree and thus has a position that is undefined.

const selected = $(e.target).val();

// When resetting the field after a successful action, there won't be a valid "selected" option.
if (selected) {
Copy link
Member Author

Choose a reason for hiding this comment

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

See the onsubmit() function - the action is intentionally reset to the default option after a successful action has been fired (self.find(':input[name=Action]').val('').change();).

This works correctly before this PR - not sure what changed but this started throwing errors on the next line because selected was null or undefined. This avoids that issue.

var version = $.prototype.jquery.split('.');
var callbackIdx = (version[0] > 1 || version[1] >= 10 ? 1 : 2);

// Monkey patch $.fn.domManip to catch all regular jQuery add element calls
Copy link
Member Author

Choose a reason for hiding this comment

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

domManip wasn't even documented - it's an internal function that was removed in 3.x so we can't patch that to see when new nodes are being added. Instead, we now use mutations.

return _domManip.apply(this, arguments);
}

// Monkey patch $.fn.html to catch when jQuery sets innerHTML directly
Copy link
Member Author

Choose a reason for hiding this comment

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

This was always a dangerous way to detect this - but thankfully it isn't necessary because setting innerHTML will result in new nodes in the mutation observer.

(function($) {

/** Taken from jQuery 1.5.2 for backwards compatibility */
if ($.support.changeBubbles == undefined) {
Copy link
Member Author

Choose a reason for hiding this comment

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

$.support is deprecated but we don't need this anyway - it's to support old browsers we don't care about.

Comment on lines -9 to -11
* Requires jQuery 1.5 ($.Deferred support)
*
* CAUTION: Relies on customization of the 'beforeSend' callback in jQuery.ajaxSetup()
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't require jQuery 1.5. It's been running on 1.7 for who knows how long, but I've now upgraded it to work with 3.6 - $.deferred is still very much supported

The note about beforeSend is unnecessary and frankly misleading here, since that customisation is included in this file anyway.

* jQuery.query - Query String Modification and Creation for jQuery
* Written by Blair Mitchelmore (blair DOT mitchelmore AT gmail DOT com)
* Licensed under the WTFPL (http://sam.zoy.org/wtfpl/).
* Date: 2009/8/13
*
* @author Blair Mitchelmore
* @version 2.1.7
* @version 2.2.3
Copy link
Member Author

Choose a reason for hiding this comment

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

This has basically just been updated with the latest copy I could find

@GuySartorelli GuySartorelli force-pushed the pulls/1/jquery3-upgrade branch 5 times, most recently from 78b96df to 9419c07 Compare October 19, 2022 00:54
Comment on lines -784 to +780
jQuery.clean( [ data ], document, fragment, [] );
$data = $(jQuery.merge( [], fragment.childNodes ));
$data = $($.parseHTML(data, document, false));
Copy link
Member Author

@GuySartorelli GuySartorelli Oct 19, 2022

Choose a reason for hiding this comment

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

jQuery.clean() is an undocumented function that was removed in 3.0 - but the functionality is effectively the same as $.parseHTML() (the latter of which does the jQuery.merge() for us, so we can skip that part).

@GuySartorelli GuySartorelli force-pushed the pulls/1/jquery3-upgrade branch 4 times, most recently from 4f3151b to c93420b Compare October 25, 2022 02:02
/**
* The URL to use for saving and loading tab state
*/
window.ss.tabStateUrl = function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This needs to be accessible in TabSet.js now

const tabset = $(this);
const tabsetId = tabset.attr('id');
const overrideState = (overrideStates && overrideStates[tabsetId]) ? overrideStates[tabsetId] : null;
tabset.restoreState(overrideState);
Copy link
Member Author

Choose a reason for hiding this comment

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

The state restore functionality has moved to TabSet.js so that each tab can be responsible for restoring its own state.
This is necessary because onAdd() is now triggered asyncronously some time after the element is added (see changes in entwine to using mutation observers) which means the tabs may not be initialised yet when this function is called.

@@ -36,6 +40,46 @@ $.entwine('ss', function($){
this._super();
},

restoreState: function (overrideState) {
Copy link
Member Author

Choose a reason for hiding this comment

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

This functionality is mostly copied directly from LeftAndMain.js with some minor changes:

  • some of the code has been slightly modernised
  • if the tabset hasn't been initialised yet, is is marked to defer restoring the state until redrawTabs() is called.

@GuySartorelli GuySartorelli force-pushed the pulls/1/jquery3-upgrade branch from c93420b to a39ede4 Compare October 26, 2022 03:12
.bind('select_node.jstree check_node.jstree uncheck_node.jstree', function(e, data) {
.on('select_node.jstree check_node.jstree uncheck_node.jstree', function(e, data) {
// Clear out namespace before triggering so entwine handlers are called
e.namespace = '';
Copy link
Member Author

Choose a reason for hiding this comment

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

This didn't need to be set to blank in the past because of a bug in jQuery where the namespace wasn't actually kept on the event after triggerHandler() was called. This bug has been fixed in the current version of jQuery, so we have to manually remove the namespace for it to get to entwine.

@GuySartorelli GuySartorelli force-pushed the pulls/1/jquery3-upgrade branch 4 times, most recently from 4986131 to 6c0deb1 Compare October 26, 2022 23:31
Comment on lines -67 to +69
ValidationErrorShown: false,
getValidationErrorShown: function() {
return Boolean(this.data('_validationErrorShown'));
},
setValidationErrorShown: function(value) {
this.data('_validationErrorShown', value);
},
Copy link
Member Author

Choose a reason for hiding this comment

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

Turning this from an entwine property into regular javascript data means it is tied to the actual DOM element, which means we can rely on it being reset when a new DOM element is added.

@@ -96,9 +98,6 @@ $.entwine('ss', function($){
}
}

// Reset error display
this.setValidationErrorShown(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

We can't rely on this being reset BEFORE we fire a validation message, since onadd is fired asyncronously to the redrawTabs() call which triggers a call to onafterredrawtabs() which relies on this value.

@GuySartorelli GuySartorelli force-pushed the pulls/1/jquery3-upgrade branch from 6c0deb1 to 3f65bd3 Compare October 27, 2022 00:52
@GuySartorelli GuySartorelli force-pushed the pulls/1/jquery3-upgrade branch from 3f65bd3 to 914b69d Compare October 27, 2022 01:07
@GuySartorelli GuySartorelli marked this pull request as ready for review October 27, 2022 01:13
@emteknetnz emteknetnz merged commit be31300 into silverstripe:1 Nov 1, 2022
@emteknetnz emteknetnz deleted the pulls/1/jquery3-upgrade branch November 1, 2022 04:27
@lekoala
Copy link
Contributor

lekoala commented Nov 24, 2022

i'm so glad the team finally decided to upgrade jquery :-) that was a pleasant surprise while reading the changelog

@NightJar
Copy link
Contributor

I have also confirmed that unclecheese/display-logic still works

@GuySartorelli I'm curious to what version you tested against.
I'm using 2.0.6 (latest 2 line, released Feb '23) and I'm seeing instant errors

Uncaught TypeError: s.default.entwine is not a function in http://localhost/_resources/vendor/unclecheese/display-logic/client/dist/js/bundle.js?m=1676431318

I'm trying to figure out if there's too much jQuery, an issue with display logic's front end bundle, or it's actually an issue with the change in interface between these two modules.

@GuySartorelli
Copy link
Member Author

I'm curious to what version you tested against.

It was way too long ago for me to remember it, and I didn't note down the version at the time, unfortunately.

@NightJar
Copy link
Contributor

I'm curious to what version you tested against.

It was way too long ago for me to remember it, and I didn't note down the version at the time, unfortunately.

That's cool, I thought that might be the case; thanks for responding.
I've been able to identify that the previously deployed display-logic version (2.0.5) still exhibits the issue, meaning it's not a regression (it worked fine in active service with CMS 4.11).

I'm busy trying to assert it's not the fault of my site before I open an issue :)
jQuery is loading, but isn't Entwine isn't binding to it for some reason, although it loads immediately afterward (in normal old school jQuery fashion).

@NightJar
Copy link
Contributor

I'm trying to figure out if there's too much jQuery


... It was debugbar 🤦
Site is fine in test/live mode 😄

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.

4 participants