Add once.js to core

A patch maybe? It looks good. Wanted to use data attributes during the rewrite of jquery.once.

This is a simple test that verifies once() is working properly. For the coverage to fully confirm that the once() replacement is working It would also need to cover once's findOnce() and removeOnce() functions.

Another patch is attached that is an attempt at replacing jquery once with droplet once. This exposed several things things that are needed before this can work:

1. The jquery shim returns vanilla JS, but needs to return Jquery objects
2. The jquery shim doesn't connect the removeOnce() function
3. Dropletonce does not have an equivalent function for findOnce(), so consequently the shim does not have an equivalent.

with the findOnce code.

Simplified the code a bit, sorry @droplet, removed the comments to make it more readable. Everything seems to be working.

once file:

(() => {
  "use strict";

  function checkId(id) {
    if (typeof id !== "string") {
      throw new TypeError("The Once id parameter must be a string");
    }
    return "data-once-" + id;
  }

  function once(elements, id) {
    const dataId = checkId(id);
    const filtered = Array.prototype.filter.call(
      elements, (element) => !element.hasAttribute(dataId));

    filtered.forEach((element) => element.setAttribute(dataId, ''));
    return filtered;
  }

  once.remove = (elements, id) => {
    const dataId = checkId(id);
    const found = once.find(elements, id);
    Array.prototype.forEach.call(found,
        element => element.removeAttribute(dataId));
    return found;
  };

  once.find = (elements, id) => {
    const dataId = checkId(id);
    return Array.prototype.filter.call(elements,
        element => element.hasAttribute(dataId));
  };

  window.once = once;
})();

once jquery file

(function ($, once) {
  "use strict";

  $.fn.once = function (id) {
    return $(once(this, id));
  }
  $.fn.findOnce = function (id) {
    return $(once.find(this, id));
  }
  $.fn.removeOnce = function (id) {
    return $(once.remove(this, id));
  }

})(jQuery, once);

( edit ) messed up the once.remove function in the patch, use the one from the comment instead.

went ahead and fixed date.es6.js which had some old school once processing. put the .es6 files to make it work on IE11 (so I can test the date polyfill)

Patch looks great. I'm so excited to see this change happening in core.

1. +++ b/core/modules/system/tests/modules/once_test/js/once-test.es6.js
   @@ -0,0 +1,17 @@
   +(($, Drupal) => {
   ...
   +})(jQuery, Drupal);
   

   I think we should add a test for once() just with Vanilla JS.

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Once/OnceTest.php
   @@ -0,0 +1,40 @@
   +class OnceTest extends WebDriverTestBase {
   

   This can be a nightwatch test now.

Bumping to major since this blocks a lot of the work in #3052002: [meta] Replace JQuery with vanilla Javascript in core.

There's a patch here :)

+++ b/core/assets/vendor/droplet-once/droplet.once.es6.js
@@ -0,0 +1,47 @@
+    filtered.forEach((element) => element.setAttribute(dataId, ''));
...
+        element => element.removeAttribute(dataId));
...
+        element => element.hasAttribute(dataId));

For my reference: is there a reason we use hasAttribute/setAttribute/removeAttribute instead of interacting directly with element.dataset ? According to https://developer.mozilla.org/en-US/docs/Web/API/HTMLOrForeignElement/da... that's supported in all the browsers we need.

I'm fixing the tests now.

Fixes the tests and adds a test-case for Vanilla use of the once library

I'm not a fan of nightwatch, so didn't touch that.

More of a jest fan myself, shameless plug: https://www.npmjs.com/package/twig-testing-library

@flxa and I spent some time debugging the fails

The issue is that droplet.once expects a NodeList but sometimes its a jquery object, and sometimes we receive the whole dom document,

flxa is working on making sure we do some casting there, we'll probably have to add a deprecation notice too.

Seems like the console error is popping up because one of the "elements" is actually the full #document. This filters that out and normalises the jQuery objects into arrays for `once`, `once.remove` and `once.find`.

I'd rather not have anything related to jQuery in the new once script, if we need to call something specific it needs to be done in the jquery.droplet.once script. the once lib should take an array-like object or an single element (todo) as input and always returns either an array or a single element as output.

Also a general clean-up is necessary, renaming files, adding comments, finding the right place to put the script because here it's in the assets/vendor directory but the code doesn't come from a github repo like the original jQuery.once script does.

Didn't use dataset because at the time it wasn't supported by all browsers we needed but you're right that we can update that too.

Yeah we came to the same conclusion
Taking those bits on

or an single element (todo) as input and always returns either an array or a single element as output.

How would this work in the single element scenario if the element was already executed

e.g. once(document.getElementById('some-id'), 'some-once').appendChild(document.createElement('div'))

In the scenario where this runs a second time, the return can't be chained.

This still to be done (if its worthwhile).

Here's the switch to a file in misc and removal of the droplet naming 😿 and license stuff.

Seeing some tests passing locally.

Interdiff is from #25

1. +++ b/core/assets/vendor/droplet-once/jquery.droplet.once.js
   @@ -0,0 +1,40 @@
   +    if (this instanceof Document) {
   +      return $(once(this.body, id));
   +    }
   +
   +    return $(once(this, id));
   

   can we avoid multiple returns?

   let el = this;
   if (this instanceof Document) {
     el = this.body;
   }
   

2. +++ b/core/misc/date.es6.js
   @@ -3,7 +3,7 @@
   +(function($, Modernizr, Drupal, once) {
   

   need to update eslint config to add the "once" variable to the global scope

3. +++ b/core/misc/drupal.once.es6.js
   @@ -0,0 +1,43 @@
   +    return id.toLowerCase().replace(/-(.)/g, (match, group) => group.toUpperCase());
   

   was missing before too but we should probably check for spaces and other unconventional characters in the id. Also like droplet said, this part is less clear than with setAttribute

4. +++ b/core/misc/drupal.once.es6.js
   @@ -0,0 +1,43 @@
   +    Array.prototype.forEach.call(found,
   

   find returns an array, so we don't need to use the array prototype

5. +++ b/core/modules/system/tests/modules/once_test/js/vanilla-once-test.es6.js
   @@ -0,0 +1,20 @@
   +      Array.prototype.forEach.call(once(document.querySelectorAll('#added-once'), 'use-once'), el => {
   

   once returns an array so we can call forEach directly on the return.

Here's a patch that

• Returns to using setAttribute/hasAttribute/removeAttribute
• Removes the old jquery.once files
• Removes the leftover droplet.once transpiled files in the last patch
• Fixes 2,3 and 4 from #36
• Introduces a set to track seen objects that are not elements. E.g. Node preview JS uses $(document).once(), off-canvas dialog uses $(window).once(). This was the source of some of the test-fails, as these objects don't support dataset or hasAttribute/setAttribute/removeAttribute. I realise there's no easy way to debug that set (e.g. via inspector) but there isn't now in those scenarios either. Manually tested off-canvas, node preview and media library and all seem to be working now.

FYI support for Set as used here is good in all the browser we're interested in - https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global...

I'd say off node is an inappropriate usage. shift them to HTML or body, can't?

Yes, but we have to maintain BC. We can fix core calls - but we can't know what things are done in contrib and custom code that will break if we force it to only use node elements. So I think we have to keep supporting that, but we need to trigger a deprecation error to force people to use it on elements instead. I think here is the place to do that too.

the non element set has a little problem, we only track the element, not the id it is called with:

once(document, "test1") => OK 
once(document, "test2") => KO empty

JS in Drupal has always been very dom-based. Because for what we do with it, the DOM is fast enough, especially these days.

One of the concern when we move to jquery once instead of using the processed class names was that information about what was processed and what was not lived outside the dom in a jquery-specific part. Having a data attribute with this information brings us back to having all the data in the DOM, I would still not tell people to use it for styling because that is not the goal (add that class in the js that call the once function), the goal is to expose the information about what has been processed in an easily accessible way*.

About the window/document issue: both should be replaced with document.documentElement because we're not doing SPA in drupal, the window == the document == the page. For SPA, just don't use window/document in the once function, easy. The deprecation can be added to the jquery extention only.

Thinking about it the initial call to attachbehaviors is wrong. Everything uses jquery so we don't see it but instead of Drupal.attachBehaviors(document, drupalSettings); it should be Drupal.attachBehaviors(document.documentElement, drupalSettings); because that's what it is that's for another issue though.

Haven't looked at the test failures. Ran eslint, updated the patch with the aliasing method, remove the Set part, started to add the jsdoc, deprecation notice need some work. interdiff is a bit rubbish because of the move but essentially we're back on the previous version of once.js, and the jquery.drupal.once file has a new method.

* I'll try to get all that in a single data-once attribute because I'm not a fan of having too many data-once attributes.

Patch that puts everything in data-once attribute. Can still use css with the ~= attribute selector for exemple: [data-once~="contextualToolbar-init"].

Also, how do we go about deprecation warning in the testbot?

Also, how do we go about deprecation warning in the testbot?

We fix the usages in core :)

All of those 38 fails are deprecation errors, so if we fix the usages in core to not use window or document this should be green 🎉

This is looking nice @nod_

We do have arguments checking in core, see Drupal.ajax :)
We should keep checkId. the function shouldn't change the id to a "safe" subset or generate one, if it's too forgiving it'll be a pain when we want to deprecate or fix a bug somewhere. I took the regex from a DOMTokenList polyfill, it follows the same rules as css class names like this.

we should fail on empty ids, you're right.

I don't want to allow selectors because then we'd need to allow passing a context element, and then it's just a shortcut for context.qSA and doesn't add anything. I'd rather keep this thing specialized and if we need some syntactic sugar on top we can put that in Drupa.whatever.

Removing with the regex leaves some extra spaces so if we add/remove a lot, we'll have a lot of extra spaces. and running a replace(wsRE, ' ') would work, but it doesn't feel right.

I prefer your code too there, though I would go with splitting an array again. Was on the fence mixing regular dom methods and browser specific implementation of reading/writting attributes.

To get around the deprecations warning, because I don't really want to touch all those modules and files we can do an argument checking in the once methods and let the aliasing on the jquery side. meanging we support using window/document when you go through jQuery but not when you use the vanilla lib. Would that be ok?

was removing the .includes() in a new patch :)

Added a little something to prevent duplicate values that would 'break' the remove function. babel add some polyfills to the script but still lighter than jquery :p

Created a stub for the change notice to have an URL to point to.

Changed a bit the patch to deprecate the existing lib instead of removing it right away. Got rid of the deprecation notice in the jquery interface because, well either we use jQuery or we don't and when we update the code to not use jQuery we'll see that using once on window/document will not work. Safer than trying to update the code of other modules in this patch. we can deprecate the jQuery interface in 9.2.x once the rest of core doesn't rely on it.
Trying to get that in rather quickly so that claro and especially olivero can use it before getting in core.

just moved the declaration of drupal.once in the right alphabetical place in core.libraries.yml

IE11... there was another issue with the .matches method, added a polyfill for it. Now it works on IE9+.

forgot the polyfill in the patch

I don't think we'll handle more than 20 items in this token list, iterating over it 2,3, or 4 time doesn't matter much compared to using the dom for filtering and storing the data.

What's left I think is:

• benchmark the new version against jquery.once
• write up the change notice
• add a change notice for the matches polyfill
• finish the jsdoc

Then we're good to go no?

Ok I see it now, I'll update the patch. Removed the foreach for updating the attributes so we go over the elements only once, not twice like before.

I'm making a benchmark on jsperf to try out a few things and to find the threshold when the perf impact of using the dom becomes an issue vs jquery.

once seems better than the jQuery once surprisingly:
https://www.measurethat.net/Benchmarks/Show/8844/1/drupal-once-variables...

I'll see if I have time to find out how many elements we use it on average.
Saw that it's not linear, we can tweak the parameters of the test in the url above to find how it behave in various situations:

• 100 divs, 5 once items
• 5 divs, 100 once items

Updated the test to query the elements in each loop :)

Thanks for the lighthouse link, never had the curiosity to look into it so it's educational. And yeah the iphone JS performance is ridiculous, so much better than any phone by far as well as most desktop.

Could use some help with review of the change notice https://www.drupal.org/node/3158256

also the library is called core/drupal.once but it's not in the Drupal namespace and I don't think it should be, might change it to core/once to reflect the actual implementation.

renamed the libs core/once and core/once.jquery. The name works this way because it's the jquery interface of the once library, not the jquery extension once. Updated the IS and change notice.

Also I changed the weight of the once.jquery file so that is it added after the jquery.once file, That way if both libraries are on the page everything uses the new library.

Now we only need the jsdoc comments for the functions.

sorry my bad had some test code leftover.

so for the styling, flashback from 6 years ago #2348321-26: Upgrade to jQuery Once 2.x, we could argue that it's a code quality issue in a specific project… But we don't want to make it easy for people to do the bad thing.

In core not all the once calls are followed by an addClass, so I would think there is often a different way of doing the styling. What would it simplify to use data attributes for styling? the JS side or the CSS side?

For the code I wouldn't use jquery for class changes but yes essentially if a class is needed I would do:

const elements = once(document.querySelectorAll('.x'), 'x')
  .forEach(el => el.classList.add('x-processed'));

For the styling I don't have a clear idea of how it should look like, just that is shouldn't be tied directly to the data-once attribute.

for the remove we can add that to the detach behavior to do:

once()
addClass('processed')
once.remove()
removeClass('processed')

first pass at the jsdoc. Need help from good english writers :)

Follow-up #3160052: Initial argument passed to Drupal behaviors is incorrect, looks scarier than it is.

That make sense, I removed the sentence about not using data-once for styling from the change record.

This provides some jsdoc revisions as requested in ##7 I did a little bit of function and variable renaming that I thought would help readability.

There was one bit I was tempted to change but didn't:

+++ b/core/misc/once/once.es6.js
@@ -0,0 +1,219 @@
+  const attrName = 'data-once';

I wasn't sure what the intent was having this as a variable since this isn't an overridable class. It seems like it would be easier for a contributor to understand if the attribute name is used directly instead of attrName, but this seemed intentional enough that I didn't want to change it.

Thank you! names are better like this agreed.

Yes in a variable is intentional, I'm not loosing hope that we'll minify core JS at some point, then it'll help… trivially :p.

Makes it easier to change it, like you did :)

Commenting the interdiff:

1. +++ b/core/misc/once/once.es6.js
   @@ -171,16 +179,17 @@
   -   *   An array-like object containing all the elements to be checked.
   +   *   A NodeList or array of elements to remove the once id from.
   

   It's a detail but array-like is on purpose, the following works and will be treated like an array (this is how a jQuery object is built btw):

   {
    0: dom0,
    1: dom1,
    2: dom2,
    length: 3
   }

2. +++ b/core/misc/once/once.jquery.es6.js
   @@ -1,11 +1,11 @@
   - * Wrapper between jQuery and core/once.
   + * Wrapper between core/once.jQuery and core/once.
   

   this is not accurate. The file is the core/once.jquery lib, it's a wrapper of core/once for jQuery

Looks much better now, updated the change notice with the new attribute name. Good to go for me :)

I'd like to keep the check element, it doesn't cost much to do the check and we make sure that people sending unexpected things have a very precise information about what's wrong. I'd keep it more for DX than correctness.

In what situation would you use once('id') without specifying the elements? If it's because of a behavior you only want run once per page, I would extend the behaviors to support this, not extend this simple library to help with this. If it's for behaviors I would

Named params, not a fan here, it does remove the order issue but it kinda makes things longer:

once({ id: 'id', elements: document.querySelectorAll('div') });
once(document.querySelectorAll('div'), 'id');

About the arguments order, no special opinion the two parameters are required (and I don't think we should default to 'html') so either way is ok with me. If we want to go with consistency, the theme functions are called with theme('themefunc', options), not really the same but similar in some ways.

In any case, to me this is good enough either way (parameter order, documentation, etc.) and I'd rather wrap up this one and move on to removing jquery dependency from ajax.

Sorry didn't read it right.

Thinking about it, I like the param order change. Makes it easier to see which "once" was called, only need to pay attention to the beginning of the line when reading the code, the messy "query" part can be ignored more easily.

in datejs for example (and document, should be context here but anyway):

once(
  document.querySelectorAll(
    '[data-…-elements="date-time"]',
  ),
  'datepicker',
)

once(
  'datepicker',
  document.querySelectorAll(
    '[data-…-elements="date-time"]',
  ),
 )

I'll probably regret this patch but here we go. The issue is that find is actually filter. So I went ahead and renamed find in filter, and created a new find function that does something more in line with what users would expect. There is no mapping in jQuery to the new function because it doesn't make much sense, nobody expect it and we don't want to encourage people using jQuery. The changes to date.js are mainly indentation, we can call forEach directly on the return of once().

I'll update the CR if we agree on those changes.

updated the CR. Who's up for RTBC?

updated the IS & some more tweaks to the CR.

And droplet should be credited first in the commit.

I feel this is ready.

We need to get things moving and this patch is needed for things to fix in the olivero issue.

The new deprecation requires test coverage.
I'm still in the process of reviewing the full patch but mentioning this now as it may be actionable before I'm able to otherwise +1 #93

Tagging with needs release note.

1. +++ b/core/misc/once/once.es6.js
   @@ -0,0 +1,259 @@
   +   */
   +  once.remove = (id, elements) => {
   +    const dataId = checkId(id);
   +    return filterAndModify(elements, `[${attrName}~="${dataId}"]`, element => {
   ...
   +   *   provided once id.
   +   */
   +  once.filter = (id, elements) => {
   +    const dataId = checkId(id);
   +    return filterAndModify(elements, `[${attrName}~="${dataId}"]`);
   ...
   +   *   provided once id.
   +   */
   +  once.find = (id, context = document.documentElement) => {
   +    const dataId = checkId(id);
   +    return (
   

   Since this is now part of core as opposed to a third party library with its own test coverage, the functions that will be called from other JS (find, filter, and remove) should get test coverage as well as the jQuery shimmed version.

   Would also be good for tests to that confirms the proper conversion of illegal spaces in once ids and how the jquery shim aliases window and document.

2. +++ b/core/modules/system/tests/modules/once_test/js/vanilla-once-test.es6.js
   @@ -0,0 +1,20 @@
   +  Drupal.behaviors.vanillaOnceTest = {
   +    attach(context) {
   +      const addedOnce = document.createElement('div');
   +      addedOnce.classList.add('added-once');
   +      addedOnce.textContent = 'Added once!';
   +      const addedOften = document.createElement('div');
   

   I see examples of test-only JS with and without docs for the behaviors. Since there's additional work needed anyway, this and jqueryOnceTest should get the usual behavior docs.

3. +++ b/core/modules/system/tests/modules/once_test/src/Form/OnceTestForm.php
   @@ -0,0 +1,65 @@
   +  public function buildForm(array $form, FormStateInterface $form_state, string $type = 'vanilla') {
   +    $form['content']['#attached']['library'][] = "once_test/{$type}_once_test";
   +    $form['trigger_message'] = [
   
   +++ b/core/tests/Drupal/FunctionalJavascriptTests/Once/OnceTest.php
   @@ -0,0 +1,57 @@
   +  public function testOnce($path) {
   +    $assert_session = $this->assertSession();
   +    $page = $this->getSession()->getPage();
   +    $this->drupalGet($path);
   +    $assert_session->assertWaitOnAjaxRequest();
   

   Especially since the $type arg for the form has a default value, the test should add an assertion that confirms the library chosen by dataprovider is actually loading. Something as simple as jquery-once-test and vanilla-once-test adding a file-specific DOM element that the test can check for.

This addresses #95.2 and #95.3

#95.1 (testing the individual methods in the new once() library) is still needed. I'm wondering if this needs to be done in a Nightwatch test or if it can added as another test in FunctionalJavascript classes we've already added.

forked and renamed droplet repo to keep the history and make some update to the code and dependencies. I think library tests should be sorted out on the library not in core #3176918: [policy, no patch] Publishing / Maintaining JS libraries produced by Drupal that do not have a dependency on Drupal.

+++ b/core/core.libraries.yml
+once:
+  version: VERSION
+  js:
+    misc/once/once.js: { weight: -19 }
+  dependencies:
+    - core/drupal.element.matches
+

If we're treating this like an external library (and it seems like that is necessary as it needs test coverage ill-suited for Drupal tests), the external repo should probably get tagged with a release and the library defintion changed, including once.js moved to assets/vendor.

Based on reviewing https://raw.githubusercontent.com/theodoreb/once/main/test/test.js it looks like some of the tests are commented out and test functionality that isn't otherwise tested. Before this would pass a dependency evaluation, those would need to be addressed + coverage for any other functions not yet covered. That is something I may be able to assist with if @nod_ can confirm the library will be continuing with that approach to testing.

Updated the gtihub repo. Release made. It's following everything we outlined in #3176918: [policy, no patch] Publishing / Maintaining JS libraries produced by Drupal that do not have a dependency on Drupal. There is a npm package: https://www.npmjs.com/package/once-dom (until we get the whole @drupal thing going).

Automated tests took something like 5 clicks to setup once "npm test" was configured and it runs test on both chrome and firefox. Code coverage is 100%.

And we have an officially published API documentation :) https://github.com/theodoreb/once/blob/main/API.md

not sure we need the drupal tests, lib is tested.

Moved the once lib to assets/vendor. Pointed to my repo, we can change to the drupal/once repo when it exists. had to reroll for the date.es6.js file, the interdiff is in the libraries.yml, the rest is updating the once lib.

I realized that the compiled version of once will not work on IE11. I'll update that, we can fix later if there is nothing else.

lib compatible with ie-11 now, good to go for me.

small update so that the version number is in the once library file.

updated the change notice with the proper attribute name and added a link to the generated documentation.

Added a release notes snippet.

I reviewed the tests in the repo and confirmed they make the tests written in this issue redundant so I've removed those. The only additional change I made was updating the Drupal version in the deprecation.

As a result most of my changes have either been removed or now live in the library itself https://github.com/theodoreb/once, which means I'm 99% comfortable RTBC-ing this (assuming something in this patch didn't mess up tests).

If @nod_ can confirm he's OK with my test removal and deprecation message change I'd consider this a sufficient co-review and RTBC. I also ran this by committer @lauriii who confirmed this would be fine.

I'm good with removing the tests :)

One side effect here is that if a project or contrib module uses core/jquery.once as a dependency, the resulting script loading will be:

<script src="/core/assets/vendor/once/once.min.js?v=2.1.1"></script>
<script src="/core/assets/vendor/jquery-once/jquery.once.min.js?v=2.2.3"></script>
<script src="/core/assets/vendor/once/once.jquery.min.js?v=2.1.1"></script>

Meaning that if one script depend on core/once.jquery, everyone uses it regardless of what they declared in their dependencies. I'm confident that won't break their code, but it does behaves unexpectedly, and it becomes very hard to use the old jQuery-once script.
So maybe to be safe we should only change olivero/date.js and not even use the new once jquery interface.

So unfortunately I think the safe thing to do is to not expose the once.jquery library (that way we don't have to deprecate it) and just update all the core code to use the vanillajs lib in a follow up, we can still get the lib in and update date.js in this patch.

Safe subset of the patch that can be committed. no more once.jquery library, only add core/once and update the date script.

I think we should use jscodeshift and a custom codemod to update the js code automatically for core/contrib/etc.

Ok so deprecation needs to happen in a followup.

updated the Change notice an opened #3183149: Deprecate jquery.once and use the new once lib

It's taking some back-and forth to find the right configuration for shipping the library files.

I added the unminifed source to make it possible for IDE to pickup the documentation and provide autocompletion for the once library. what's loading is still the minified file.

now i'm done

#117 looks very good. My only concern is that the only actual use of the new library is something that would be completely ignored by tests since it requires a condition that won't be the case in our tests. Although I'm quite confident this new library works fine within core, I think it would be good to refactor $().once to once() for an asset that is used in existing test coverage. Most of this would obviously happen in #3183149: Deprecate jquery.once and use the new once lib, but I like the practice of providing at least one common-use change with updates like this. This would be a test-confirmed assurance that such a change does not result in some unexpected conflict with Drupal.

This also needs a dependency evaluation and issue summary update since the approach slightly changed. I'll take care of those.

how about every call updated #3183149-4: Deprecate jquery.once and use the new once lib :D

like droplet suggested might need to do the querySelectorAll inside the once call. It's a lot of noise to have that everywhere. Or better have a separate function that we can use as shortcut across core, or even

(function (qsa) {

  once('something', qsa('[data-drupal-selector]'))

}(document.querySelectorAll.bind(document)));

there is a test for claro autocomplete, so I changed that one.

The test in #121 serves as a nice confirmation that there isn't a mysterious breaking change going on.

The querySelectorAll change mentioned in #120 is probably more appropriate for #3183149: Deprecate jquery.once and use the new once lib, so I'll go ahead and RTBC. I am +1 on the noise reduction, though.

Much of this patch has moved to the library or #3183149: Deprecate jquery.once and use the new once lib. The result is this becomes much easier to review, and a more manageable scope overall, so this is good!

Some improvements to the library https://github.com/theodoreb/once/pull/3 (I should just do everything droplet tells me from the start :p)

Doesn't change anything for this issue, it'll be useful mainly the deprecation issue, but no reason to hold up this patch on that.

Haven't reviewed the patch yet but since we're adding a new dependency, we need sign-off from release managers.

Update with latest version, makes calls to once shorter and more readable.

small upkeep fixing some inconsistencies in the lib and make a smaller minified file.

I don't think it's a problem to keep the possibility of using a context, it's an optional parameter. In that example I'd do:

once('media-library-menu-item', '.js-media-library-menu a')

Allowing a jQuery object as a context makes us run into the issue I show at the bottom of #3183149-23: Deprecate jquery.once and use the new once lib so I'd rather not do that.

( edit ) having the context will help with the transition. We can always deprecate it later

Update of the lib.

I'm expecting to go back to 1.0.0 when the lib moves away from my github fork, so I'm not too worried about having that many changes to the version at this point. It'll be a different npm package when we move that away from my fork.

And about the context i think we need to keep it. It's useful for ajax forms to know what to serialize/unserialize because context will be the part of the form that is targeted.

Also I think that the targeted context might not always be attached to the DOM, meaning that document.querySelectorAll() can't find it and we need to be able to do context.querySelectorAll to be able to target elements in a detached dom node.

Thanks everyone for your work on modernizing this dependency!

I've discussed the dependency addition proposed here with @catch, @gabesullice, @bnjmnm, @Dries, and @lauriii. This issue raises some interesting governance and maintenance questions.

#3176918: [policy, no patch] Publishing / Maintaining JS libraries produced by Drupal that do not have a dependency on Drupal is definitely relevant to this issue. The library does not need to depend on Drupal, and it would benefit from being in a separate repository for many of the same reasons the proposed Drupal JS client libraries would:

1. Publishing to npm
2. Easily setting up custom testing workflows
3. No having to run the entire hour-long PHP core test suite for every WIP change.

Based on that, I think Once should be treated as a independent Drupal Core JavaScript package as described in #3176918: [policy, no patch] Publishing / Maintaining JS libraries produced by Drupal that do not have a dependency on Drupal (even given that its equivalent was an independent project maintained by individual Drupalists on GitHub).

However, the current policy draft is not finalized, and some important decisions about governance and the code origin are still not settled. So, I think it would be best to make this an independent package once we've settled that policy.

That said, neither @catch nor I want to see progress here blocked on the final decisions of the JavaScript package work. (As @catch pointed out, the point of #3176918: [policy, no patch] Publishing / Maintaining JS libraries produced by Drupal that do not have a dependency on Drupal is to accelerate Drupal JavaScript development, not to bog it down.) Given that, and given that this is going to be a production dependency of a production version of core, our proposed compromise is:

1. For now, just add Once as a core library, rather than an external dependency. I reached out to @bnjmnm about how the test suite could work if the library were in core. Ben confirmed that they could be converted to Nightwatch tests, and is willing to implement this conversion if we go ahead with this as a core library.
    

2. File a followup issue to make it one of the new packages covered by #3176918: [policy, no patch] Publishing / Maintaining JS libraries produced by Drupal that do not have a dependency on Drupal once we've established those policies, so that it can benefit from those same decisions and features (hopefully soon!).

Sorry that this means more work, as well as the overhead of core tests and queue, etc. It should be a temporary situation until #3176918: [policy, no patch] Publishing / Maintaining JS libraries produced by Drupal that do not have a dependency on Drupal is resolved and we can move the project to its own repo and publish the library on npm.

I'm leaving the RM review tag on because we'll want to review it again if we go ahead and implement the compromise.

Thanks for the update. Given that, for now I would keep it as an external library and revisit the "core" status of the lib once the policy is agreed upon. The cost is way too high for what it is in this situation. We could name this library "core/once-dom" to map with the npm package name and keep the "core/once" namespace open for the "real thing".

Putting it in core will take time to figure out how to integrate with core tests, but there is also the build step to worry about. I'm using rollup and buble because they're very easy to use and there is no need for endless configuration, but that means we'd have to do a dependency audit for those and it's not going to be fun.

On my end I already spend a good amount of time on this to get the lib + updating all the core code, given that we have 2 green patches that put the lib and update all core code, it's going to be hard to justify spending more time on this to my employer, especially if it's for redoing the same work with tools that are not suited for the task. So I'm happy to reroll things, agree to keep the once lib up to date until it's under official core status, but I can't spend more company time on the compromise scenario and I won't spend personal time on this.

(also I'm much more worried about the second patch, so if there is anything that can help get it through happy to help. to that end I got the test coverage of all the JS functional tests if that can reassure people)

Does a working example qualify as test? :)

((Drupal, once) => {
  function init(el) {
    var background = el.querySelector('div[data-type="background"]');
    window.addEventListener('scroll', function () {
      var c = -(window.scrollY / background.dataset.speed),
          n = "center " + c + "px";
      background.style.backgroundPosition = n;
    });
    var c = -(window.scrollY / background.dataset.speed),
        n = "center " + c + "px";
    background.style.backgroundPosition = n;
  }

  Drupal.behaviors.scrollingHero = {
    attach(context) {
      once('scrollingHero', context.querySelectorAll('.region--hero')).forEach(heroRegion => init(heroRegion));
    }

  };
})(Drupal, once);

@viappidu something like this yes, it would be to make sure drupal load the library properly, that it can be accessed from "drupal-style" javascript, and that it does what is expected.

As a sidenote you can simplify the code like this

once('scrollingHero', '.region--hero', context).forEach(init);

Just to confirm, @bnjmnm is definitely working on the Nightwatch version of the test suite. It's understandable if @nod_ and @droplet can't put additional work in on that front at this time. When @bnjmnm posts that patch, we can compare the two approaches and discuss with Dries as needed.

This adds nightwatch equivalents of the tests in the library https://github.com/theodoreb/once/tree/v3.5.0/test. It is also moved to misc as it's not being treated as a vendor library at the moment (but hope to see soon 🙂)

Nightwatch isn't the ideal way to do these tests, nor is it ideal to rewrite existing+working tests for a different tool. I also think the work @droplet and @nod_ have done here is great and if getting it into core means adding some weird Nightwatch tests I'm happy do do them.

1. +++ b/core/misc/once/once.es6.js
   @@ -0,0 +1,375 @@
   +// eslint-disable-next-line func-names
   +window.once = (function () {
   

   We can use an arrow function so we don't have to add an eslint-disable comment.

Checked the tests, they work and cover the same thing as the lib, so the code itself is RTBC for me.

My main question if we go this way, how the switch to a "properly" managed library will look like?
We had issues with JS file paths before that prevented some cleanup #2484623: Move all JS in modules to a js/ folder. So are we going to keep the core/once namespace for it and just swap the lib (and file path) or are we going to deprecate core/once and introduce a new library (core/@drupal/once? to follow the @drupal/once npm package name it'll be under?) and ask people to update their code again in the next release?

Very good point in #140. I wonder if it would be possible to have this initial implementation be in core/assets/vendor as well. Here's something that came to mind that is odd, but as far as I can tell has fewer tradeoffs than switching js paths or creating two different libraries.

• Everything in the approach that isn't 100% standard will be heavily documented with comments and readmes. This would be deviating from exact policy a bit, but far less than jQuery once!
• The source can remain in core/misc, but we add build tools to compile it core/vendor/once, which also provides the benefit of minification. core/assets/vendor/once will have a README explaining that the source is coming from core, not an external.
• In core.libraries.yml, it would be configured as a core library, with comments making it clear that the file being loaded from core/vendor is based on source in core/misc

There may be glaring caveats I overlooked in this proposed approach, but perhaps some version of this could work?

We briefly discussed this issue on today's JS Menu Component initiative discussion with the DA and committers. Based on that discussion and feedback from nod_, I think we might want to attempt to adopt the "interim decision" for this. The MVP for standalone projects on d.o is:

1. New project type
2. Publishing to npm from that new project type

Per nod_, we can do everything else the hacky way (running the build steps locally, running tests with not-the-bestest-tools) so long as it's on npm as we intend, and then fix those things later. From a release management perspective, I'd love to have the initial commit be under the official Drupal namespace on npm as a stable initial release for us to commit it to core (so we don't have to change that later).

I'll reach out to @hestenet and ask about how long those initial steps might take. If it's months and months, we should maybe go ahead with @bnjmnm's patch, but if it's "We can have that for you in two weeks" then it might be worth using Once as the test case. Inaugurating a new tag for the feedback.

We might need to wait after the holidays for DA feedback (for a timeline on adding a JS project type to Drupal.org, as well as some gitlab CI access and setup for secure publication to the @drupal namespace on npm). It might not be in 2020 but it won't take years either. Maybe you haven't seen the news yet droplet #3187489: Add @justafish and @nod_ as core committers to MAINTAINERS.txt, so that should help keeping things moving :D

details .details-title // Claro
#views-tabset a // views
div#views-live-preview a // views
div#views-live-preview input[type=submit] // views
#system-theme-settings .color-form // color module

And that's it, the rest of the selectors in once don't have a hierarchy "condition". It's selecting based on the class, data attribute, tag name, or id.

Small update: DA is taking actions around this topic as a priority, if all goes well we'll have everything this month. That will unlock a lot of things, this issue included.

DA just released the new project type #2474007: Add a “General” project type to Drupal.org on d.o. The Gitlab part is also on the way, so good news all around :)

I went ahead and created: https://www.drupal.org/project/once I'll push the repo tomorrow as it's getting late here.

The once lib is now on NPM :D. We have a new project type and a pipeline that published code to npm under the @drupal namespace automatically, removing the tag for DA review.

updating the patch with latest code. For making use of the npm package itself in core see #3185289: Use package.json and rollup to manage third party JS libraries

updating the change notice with links and mention of the npm package.

Retitleing and removing the 'Needs release manager review' tag now that it's decided exactly where the new library lives.

Fixed CI issues.

I'd say go for it, I don't see outstanding problems or concerns.

We'll need to have some integration tests to make sure the once lib works as intended for drupal core but we can do that in the follow-up issue where we replace everything in core, this one is long enough already.

@droplet confirmed it with the other committers, no hidden issue that needs approval.

Nice find, fixed.

Looks great! +1 🎉

Added a release note. This will probably be a highlight too.

Let's tag the 1.0.0 release of https://www.npmjs.com/package/@drupal/once before we commit this to HEAD. We need to add a stable version semver contract before core adds the code -- otherwise, it adds a release blocker for the release of 9.2, which isn't something we allow for minors.

If we run into any issues in #3183149: Deprecate jquery.once and use the new once lib that require public API changes to the library, we still have until the 9.2.0-beta1 commit deadline to release a 2.0.0 version of once.js, and 1.x can be EOLed in that situation. (The restrictions around version support etc. only kick in when the library ships in a stable version of core, which will be 9.2.0 in this case.)

#159 means we're blocked here on #3195360: Stable 1.0 version

Thanks for the details on version support. I'll start the dependency evaluation so that we can have a stable version by monday hopefully, the code that we bring in to core doesn't have dependencies, the evaluation is for dev dependencies.

I'm not anticipating any API change (or any code change) at this point, the patch in #3183149: Deprecate jquery.once and use the new once lib is already complete and green. I'd be surprised to find an edge case we didn't already run into.

built the 1.0.0 version locally, it'll be the same as the one we'll publish later. I'll update the patch again if necessary

non minified is for IDE integration, that way there is documentation when using the once lib.

But agreed that it's not 'clean' as far as the source mapping is concerned.

source map issue has been fixed, only need a release now :)

I'm concerned this doesn't have any integration tests. The coverage of the library itself is excellent, but I think there should be some minimal coverage to confirm it is properly integrated with Drupal. I ran tests on a version of the #162 that removed the usage of once() from datepicker and Claro autocomplete, and the tests passed without error, but both would be scenarios that caused multiple instances of behavior that should only occur once. https://www.drupal.org/pift-ci-job/1991107. An abbreviated form of the core tests that were present as recently as #105 would work fine.

Integration testing is going to happen, I wanted to do that in the next issue where we deprecate the jquery.once lib because this issue is long enough as it is.

We have a 1.0.0 release of @drupal/once :D

interdiff against #154 as it was the last RTBC. The diff in size is because the map file doesn't include the source inline.

To test you can edit any node with the claro admin theme, and do a once.find(); in the console to see the couple of fields that have been marked as processed (the author field is always there).

reroll after #3113649: Remove drupal.tabbingmanager's jQueryUI dependency (confilict in .estlintrc.json file)

Creditting @catch, @gabesullice, @bnjmnm, @Dries, @lauriii, @xjm for #132

I fixed #169 / #171 locally since the conflict was in core/.eslintrc.json and I run the custom commands locally prior to committing anyway.

I noticed that this patch had test coverage but that was then removed. I checked to ensure the once library has test coverage and it does - https://git.drupalcode.org/project/once/-/tree/1.0.x/test

Committed 5bce5ee and pushed to 9.2.x. Thanks!

That one makes me very happy, thank you everyone :)

Congratulations folks

#3183149: Deprecate jquery.once and use the new once lib is complete, green, and in need of review, then we can say goodbye to jquery.once :)

This issue is a critical part of D10 development.