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

Command icon 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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_ created an issue. See original summary.

nod_’s picture

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.

nod_’s picture

Issue summary: View changes
nod_’s picture

Issue summary: View changes

nod_’s picture

Status: Active » Needs review

still needs tests, comments, etc. but it's a start.

From the Issue summary what happens is:

// Expected
$(el).once('test1')// => 1 element
once('test1', el)// => 1 element => KO

once('test2', el)// => 1 element
$(el).once('test2')// => 0 element

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.

nod_’s picture

Issue summary: View changes

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

nod_’s picture

Added some comments and nightwatch test for the BC feature and basic once lib integration tests.

nod_’s picture

Issue summary: View changes
nod_’s picture

nod_’s picture

Updated the change record to add paragraph about BC.

nod_’s picture

git fail, had to make a new MR.

bnjmnm’s picture

Status: Needs review » Needs work

Setting to NW for visibility, but this is pretty much nits and I like the approach here.

nod_’s picture

Status: Needs work » Needs review

The split between issue queue and MR is confusing. updating status.

nod_ credited jptaranto.

nod_’s picture

adding credit from the previous merge request. Mapping to the d.o user name didn't work for some reason.

bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

All 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 .

nod_’s picture

Title: Figure out BC for jquery once by @drupal/once » Add BC layer between @drupal/once and jQuery.once
lauriii’s picture

Status: Reviewed & tested by the community » Needs work

Posted some feedback on the MR

nod_’s picture

Status: Needs work » Needs review
bnjmnm’s picture

Issue summary: View changes
nod_’s picture

Issue summary: View changes
bnjmnm’s picture

Status: Needs review » Reviewed & tested by the community

I 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

  • There was a request to update the issue summary to specify that calls to jQuery.once would not update the @drupal.once registry. Although this was documented in the "Proposed Resolution" section, I did some rephrasing to make that more evident.
  • There was a question regarding the presence of Nightwatch tests for something that may be more suited for testing within the library, and I agree with @nod_ that while the library itself has thorough testing, this additional integration testing is good to have (and actually I requested in #2402103: Add once.js to core, so pleased to see it here.

  • lauriii committed 213a565 on 9.3.x
    Issue #3207782 by nod_, bnjmnm, jptaranto: Add BC layer between @drupal/...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed 213a565 and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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