Problem/Motivation
If code relies on a core jQuery.once call, with the move to @drupal/once that code might execute more than necessary
Steps to reproduce
This can happens is a piece of code in contrib relies on a call to once from core (or another module that switched to @drupal/once)
// Before
$(el).once('test')// => 1 element
$(el).once('test')// => 0 element
// Problem
$(el).once('test1')// => 1 element
once('test1', el)// => 1 element => KO
once('test2', el)// => 1 element
$(el).once('test2')// => 1 element => KO
// Expected
$(el).once('test1')// => 1 element
once('test1', el)// => 0 element
// OR/AND
once('test2', el)// => 1 element
$(el).once('test2')// => 0 element
Proposed resolution
Support ONLY
On pages where jQuery.once is loaded, calls to @drupal.once will also update the jQuery.once registry.
This covers the use case of core updating to using @drupal/once, but a module is still using jQuery.once and has expectations regarding the contents of the jQuery.once registry.
once('test2', el)// => 1 element
$(el).once('test2')// => 0 element
Calls to jQuery.once will not update the @drupal/once registry
This would effectively be providing two-way compatibility instead of backwards compatibility. A module using @drupal.once should not have functionality that depends on jQuery.once usages or its registry. jQuery.once and @drupal.once can still be used simultaneously - but @drupal.once will intentionally not take jQuery.once's registry into account when determining if something has been "onced". After updating code to use @drupal.once, it should be confirmed any reliance on other module's use of "once" are ones also using @drupal.once.
$(el).once('test1')// => 1 element
once('test1', el)// => 0 element
In this case work with upstream to make them use @drupal/once or stick with using jQuery.once() until it is updated.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3207782
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- once-bc-jquery changes, plain diff MR !874
- 3207782-figure-out-bc changes, plain diff MR !516
Comments
Comment #2
nod_Main thing I think is to agree on who should do extra work? should jquery.once call @drupal/once or should calls to @drupal/once also make calls to jquery.once
I'd go for the second, less messing about with window/document.
Comment #3
nod_Comment #4
nod_Comment #6
nod_still needs tests, comments, etc. but it's a start.
From the Issue summary what happens is:
if people use jquery to do the once, using @drupal/once won't see it, and I think that is ok here. The main issue I see is that core would use @drupal/once, and contrib breaks because jQuery().once() doesn't see it's already been "onced", patch fixes that use-case.
Comment #7
nod_Comment #9
nod_Added some comments and nightwatch test for the BC feature and basic once lib integration tests.
Comment #10
nod_Comment #11
nod_Comment #12
nod_Updated the change record to add paragraph about BC.
Comment #15
nod_git fail, had to make a new MR.
Comment #16
bnjmnmSetting to NW for visibility, but this is pretty much nits and I like the approach here.
Comment #17
nod_The split between issue queue and MR is confusing. updating status.
Comment #19
nod_adding credit from the previous merge request. Mapping to the d.o user name didn't work for some reason.
Comment #20
bnjmnmAll the feedback from my line-by-line review has been addressed. The Nightwatch tests cover the use cases needed for assurance BC is properly supported, and my manual testing in the browser console was consistent with that. In instances where the jQuery Once library is loaded, calls to @drupal/once simultaneously update the jQuery Once registry. This means that modules with expectations regarding core's use of jQuery once will still have those expectations met, even if core is "once-ing" with @drupal/once .
Comment #21
nod_Comment #22
lauriiiPosted some feedback on the MR
Comment #23
nod_Comment #24
bnjmnmComment #25
nod_Comment #26
bnjmnmI went through the additional feedback items that appeared after my first RTBC. I confirmed every closed item was correctly switched to that status, and the two not-closed items are addressed in my opinion
Comment #28
lauriiiCommitted 213a565 and pushed to 9.3.x. Thanks!