jQuery Once 2.x comes with:

  • Performance improvements due to its change from manipulating element classes to using .data()
  • Its encouragement of using best practice jQuery chaining
  • A new .findOnce() function to find once'd instances
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

RobLoach’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
25.97 KB
nod_’s picture

Component: asset library system » javascript

Will have a look

nod_’s picture

Issue tags: +JavaScript clean-up
FileSize
28.16 KB
2.38 KB

On performance On the views ui page I go from 5.1ms spent in .once() to 1.2ms, definitely a big improvement. New code also means we don't need sizzle to run jquery once, that's very much in line with #1541860: Reduce dependency on jQuery.

There were a few spots missing from the conversion, see interdiff.

I browsed around, couldn't find anything broken. Only the collapse polyfill for details is still using collapse-processed class (which is now explicitly added in the JS so it's fine). Still works as expected.

@Rob, what's left for a stable 2.0 release?

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Updated the update/jqueryonce branch on my own fork and it applied cleanly. Looks good to me.

@Rob, what's left for a stable 2.0 release?

What would you like to see in there before a 2.0.0 release is live? Thanks for taking part in the jQuery Once issue queue, by the way.

nod_’s picture

Making the id required, but I don't feel too strongly about it.

RobLoach’s picture

FileSize
590 bytes
28.74 KB

Missed the version number update in core.libraries.yml.

RobLoach’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
28.74 KB
590 bytes
RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs manual testing

Hey, Rob. :) Nice to see you in the core queue again.

Ehhh hold on a sec. Why are we pulling this from github.com/robloach and not http://plugins.jquery.com/once/?

+++ b/core/assets/vendor/jquery-once/jquery.once.js
@@ -1,92 +1,131 @@
- * jQuery Once Plugin 1.2.3
- * http://plugins.jquery.com/once/
...
+ * jQuery Once 2.0.0-alpha.8
+ * http://github.com/robloach/jquery-once

Not a huge fan of this change. Especially since plugins.jquery.com points you over to your github repo, I would prefer to keep the jquery.com domain as the source.

If that's not possible, then we have to move this file from /vendor/jquery-once to /vendor/robloach but that just feels weird. :)

Separately, it'd be great if we weren't going from a stable release to an alpha. It's not a huge deal since D8 is only in beta atm, but if a stable release is really only 1 issue away, it might make sense to postpone this for a week or so, so we don't end up dealing with it twice. The improvements look nice, but not really fixing any bugs per se.

And finally, there are half a dozen or so libraries in core.libraries.yml that declare jquery.once as a dependency. Can we get verification that there was manual testing done (e.g. the installer, collapsible fieldsets, machine names all look like possible places that could break).

RobLoach’s picture

Hey, Rob. :) Nice to see you in the core queue again.

I've missed you too. Taking a break from Drupal core gave me some huge insight into how projects outside the Drupal-verse develop and grow. It's great to see Drupal core continuing to take strides in that direction. Now that beta is out, it reminded me how much I am actually apart of this community. My involvement helps push both the project and the community forwards, so I'm happy to be back.

If that's not possible, then we have to move this file from /vendor/jquery-once to /vendor/robloach but that just feels weird. :)

JavaScript assets/components don't have vendor namespaces; Only PHP libraries installed with Composer do that. jQuery Once will continue to live in vendor/jquery-once.

http://plugins.jquery.com/once/ vs http://github.com/robloach/jquery-once

Glad you mentioned this, as it's something I had been considering myself. http://github.com/robloach/jquery-once is the actual homepage of the project, which has all the useful documentation a user of the project would want. http://plugins.jquery.com/once/ is just the jQuery plugin registry that just brings in that information. While http://plugins.jquery.com use to host code, and actually be the home for jQuery Once, it is no longer that. jQuery Once now lives on at http://github.com/robloach/jquery-once . I've opened up a Pull Request to switch the homepage to jQuery Plugins registry, let's discuss over there.

Separately, it'd be great if we weren't going from a stable release to an alpha.

Agreed, and this is why I opened this issue, so that we could get more feedback before a stable 2.0.0 goes out.

it might make sense to postpone this for a week or so

Postponing a week will only postpone it a week. If there's no actions we're taking during that time, nothing will be gained. If, however, we create issues in the jQuery Once queue and participate there, then we'll have a direction to take, and know where to go. So, this is why I pushed this forwards, so that there's some good feedback (thank you), and we know where to go with it.

RobLoach’s picture

nod_’s picture

Not sure about the change in editor.js otherwise this looks good.

@webchick: This should be committed early. All the important API changes have been made in jquery once, what's left for a stable release is not API-Related (or at least I'm very happy with the API as it is now at least).

nod_’s picture

Issue tags: +Needs manual testing

nod_ queued 12: 2348321-12-jqueryonce.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 12: 2348321-12-jqueryonce.patch, failed testing.

droplet’s picture

@RobLoach,

Could you provide a Plain JavaScript version of jQuery.once ?

RobLoach’s picture

Status: Needs work » Needs review
FileSize
28.47 KB

Updated to jQuery Once 2.0.0-beta.2 on the update/jqueryonce branch. git diff 8.0.x update/jquery

Could you provide a Plain JavaScript version of jQuery.once ?

Plain JavaScript version? The source and build is available at https://github.com/robloach/jquery-once .

droplet’s picture

@RodLocah,

Remove jQuery dependency.

RobLoach’s picture

Remove jQuery dependency.

jQuery Once uses a few things from jQuery to make it work, filter and data namingly. Not saying it wouldn't be possible, just a bit out of scope for this issue. Let's discuss over at https://github.com/RobLoach/jquery-once/issues/32 .

droplet’s picture

few drawbacks of version 2:

  1. jQuery.data reads HTML5 data-* Attributes but it won't write back. If we preset data-* attributes , there's inconsistent after jQuery.once calls.
  2. Everywhere in D8 using data-* attributes, I don't see any reason to use jQuery.data here. Searching in D8, there's 125 results of jQuery.once calls. Assumed that for a large project, it will be 10x of jQuery.once usage, it's 1250 calls. Does it really a performance issues ? Based on this jsperf (http://jsperf.com/dataset-vs-jquery-data), 1xxx calls spent around 1ms ?
  3. Additional to above points, it's less developer friendly, we could not see the jQuery.once things in Source code. (I meant when you read the HTML source core, not finding in DOM properties)
  4. No standard way to style processed elements. Adding Class via jQuery.addClass() that will make one more rule in Drupal Standard docs. it's not a good API & practice for Drupal Core ??
RobLoach’s picture

1. jQuery.data reads HTML5 data-* Attributes but it won't write back. If we preset data-* attributes , there's inconsistent after jQuery.once calls.

Correct, it does not manipulate the data-* attributes. The reason for this is so that the browser won't have to re-render each time a .once() method is called, using .data() rather than .addClass(), giving us a performance improvement.

2. Everywhere in D8 using data-* attributes, I don't see any reason to use jQuery.data here. Searching in D8, there's 125 results of jQuery.once calls. Assumed that for a large project, it will be 10x of jQuery.once usage, it's 1250 calls. Does it really a performance issues ? Based on this jsperf (http://jsperf.com/dataset-vs-jquery-data), 1xxx calls spent around 1ms ?

Since it doesn't manipulate the DOM attributes, it'll result in improved render performance. Just like the article you referenced. Adding a class for each of those 1250 calls means manipulating the DOM 1250 times. jQuery Once 2.x avoids manipulating the DOM 1250 times through using .data instead of .addClass.

3. Additional to above points, it's less developer friendly, we could not see the jQuery.once things in Source code. (I meant when you read the HTML source core, not finding in DOM properties)

To find jQuery.Once things, use the .findOnce() function.

// Find initial set of droplets
$(".droplet").once("droplet").each(doSomething);

// Finds the same set of droplets as the above call
$(".droplet").findOnce("droplet").each(doSomethingElse);
4. No standard way to style processed elements. Adding Class via jQuery.addClass() that will make one more rule in Drupal Standard docs. it's not a good API & practice for Drupal Core ??

jQuery Once's mission is to only act on elements once, not style elements. If there is a need to change the style of the elements, then that's addClass()'s job.

// Trigger Droplet behaviour
$('.droplet').once('droplet').each(function() {
  alert("Acted on Droplet!");
}).addClass("dropletted");

// CSS for Droplets that have been Dropletted
.droplet.dropletted {
  color: red;
}

This means we don't make assumptions on what people will do with it. It simply acts on elements once, that's all. Check collapse.js for an example where we use addClass() in addition to once().

droplet’s picture

FileSize
162.07 KB
Since it doesn't manipulate the DOM attributes, it'll result in improved render performance. Just like the article you referenced. Adding a class for each of those 1250 calls means manipulating the DOM 1250 times. jQuery Once 2.x avoids manipulating the DOM 1250 times through using .data instead of .addClass.

A quick performance tests ( see attached test case ):

This is 12,000 calls

Chrome:
droplet mod: 462.909ms
original: 434.218ms
(Chrome given different result on each load, sometimes droplet mod is faster)

diff: 28.691

Firefox
droplet mod: 464.92ms
original: 346.06ms

diff: 118.86

iPhone 5s / iOS8
droplet mod: 622.887ms
original: 1853.291ms

jQuery.once is faster on Desktop

This is 1,200 calls

Chrome:
droplet mod: 21.020ms
original: 53.823ms

Firefox
droplet mod: 33.11ms
original: 46.38ms

iPhone 5s / iOS8
droplet mod: 62.469ms
original: 165.849ms

Droplet Mod is faster all time

** Note that: 1200 calls is a very crazy assumption already. I think on average it's less than 30 calls. (It won't executed all Once function at same page.)

To find jQuery.Once things, use the .findOnce() function.

You missed my point.

jQuery Once's mission is to only act on elements once, not style elements. If there is a need to change the style of the elements, then that's addClass()'s job.

Yes, I can understand it. That's why I try not post it to Github. But when a project used on Drupal, I think we need some extra work (An custom API for this standard theming rules).

+++ b/core/misc/collapse.js
@@ -98,7 +98,7 @@
+      var $collapsibleDetails = $(context).find('details').once('collapse').addClass('collapse-processed');

For example this changes, I wonder if I named it with "collapsed", would maintainers make a complaint? Would developers submit new issues to every modules page ?

nod_’s picture

I agree that a no-jquery version of this script would be better. That said, we need another issue to discuss this, also dataset is not supported by IE10 and below.

With the 2.0 version there are significant perf improvements already. We can always do better, sure.

As for the usage of once, on the frontpage when logged in as uid 1 there are 27 calls to once. We're calling it whether elements were processed or not, so an ajax call would double the use for it (that's what happens on the front-page actually).

Open up a view, 88 calls to once.
Click on add field: 61 more calls.
Now just add 1 field: 203 more calls.
Click "apply" on the field modal: 185 calls.

Doing all that: 537 calls in total to .once. 1200 might be a bit high but 30 would be too low for prod websites. We should also talk about getting rid of some behaviors or doing some optimisations. Once again, another issue.

droplet’s picture

Hmm.

Thanks nod_. You made me think more on it. I changed the test cases more close to real world usage. I surprised at the performance improvements (I may really missed something on my script)

Check it out:
#2402103: Add once.js to core

catch’s picture

Priority: Normal » Major
Issue tags: +front end performance

I've seen front end performance issues on real sites due to .once() calls. The performance issue is not with the DOM manipulation itself but the browser re-rendering as Rob points out. This is the whole reason to switch to -data attributes from classes really.

Not sure about this being critical since there's no release at all for the 2.x branch yet, so splitting the difference at major for now. If there was a release candidate then it'd fall under #2203431: [meta] Various asset (JavaScript) libraries have to be updated to a (minified) stable release prior to 8.0.0.

droplet’s picture

Nope. Not everything would trigger expensive rendering performance issues, trigger re-rendering & reflow.

For example in this issue page running jQuery.once verion 1.0, opening Console and execute:

jQuery('*').once('no-css-rules-for-this-class');

Any re-rendering performance issue do you noticed? (does it freeze browsers or?)

I think the main performance in real sites ARE NOT $.once calls itself. That's caused by callback functions of $.once.

Also, in $.once version 2 is not using -data attributes. that's $.data().

YesCT’s picture

making tag the more common one so we can delete the lesser used one so it does not show in the autocomplete

RobLoach’s picture

FileSize
29.93 KB

This issue is simply about updating jQuery Once in core. Let's keep the performance tweak discussions over at https://www.drupal.org/node/2402103 or https://github.com/RobLoach/jquery-once/issues/32 .

This patch...

The branch is in a pull request here: https://github.com/RobLoach/drupal/pull/2

nod_’s picture

+++ b/core/modules/ckeditor/js/ckeditor.admin.js
@@ -55,7 +55,7 @@
+      var $configurationForm = $('.ckeditor-toolbar-configuration', context).findOnce('.ckeditor-configuration');

we still want to keep $(context).find()

Rest looks good.

RobLoach’s picture

we still want to keep $(context).find()

Hmm? There is no more .ckeditor-configuration-processed class, so using .find() will not be enough. Hence the use of .findOnce() there.

https://github.com/RobLoach/drupal/pull/2/files#diff-6975a5b9a36a4033572...

Seem like if anything, we would want .removeOnce() there, since we're detaching the ckeditor. Not sure how Drupal.ckeditor.models functions in that case though.

nod_’s picture

What I meant was before the findOnce:
$('.ckeditor-toolbar-configuration', context).findOnce('.ckeditor-configuration');

This is the format we use everywhere else in core:
$(context).find('.ckeditor-toolbar-configuration').findOnce('.ckeditor-configuration');

RobLoach’s picture

FileSize
29.94 KB

Ah, good catch. Interdiff.

jibran queued 33: 2348321-33.patch for re-testing.

rteijeiro’s picture

Status: Needs review » Reviewed & tested by the community

Looks good! Is there anything else to do here?

nod_’s picture

Rechecked, it's all good. RTBC++

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a major task that will improve performance and the disruption it introduces is limited. Per https://www.drupal.org/core/beta-changes, this is a good change to complete during the Drupal 8 beta phase. Committed cbd0184 and pushed to 8.0.x. Thanks!

  • alexpott committed cbd0184 on 8.0.x
    Issue #2348321 by RobLoach, nod_: Upgrade to jQuery Once 2.x
    
jibran’s picture

Title: Upgrade to jQuery Once 2.x » [Change notice] Upgrade to jQuery Once 2.x
Status: Fixed » Active
Issue tags: -Needs manual testing +Needs change record

Can we please have change notice for this? I have some contirb modules which might need JS update and I don't know where to start.

RobLoach’s picture

Status: Active » Needs review
alexpott’s picture

Title: [Change notice] Upgrade to jQuery Once 2.x » Upgrade to jQuery Once 2.x
Status: Needs review » Fixed
Issue tags: -Needs change record

CR exists - so we're good here now.

jibran’s picture

Thanks @RobLoach that's awesome.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

MustangGB’s picture

Status: Closed (fixed) » Needs work

2.0.1 released

nod_’s picture

Status: Needs work » Closed (fixed)

Please open a new issue and make the meta a parent. We try to avoid reopening issues like this because it can be confusing if the update introduce a bug we have to fix in our code.