window.once name conflict with other third party script

Thanks for linking. You're right, it's pretty clear that forEach is intentional. It's possible that something is wrong with our env specifically, so I'd be interested to hear if anybody else is encountering this.

That regex did work, but actually did not fix the problem. Instead I started getting once().each is undefined instead, so something else is going on. Updated the issue description.

Alright, updated the issue again. This is not related specifically to forEach(), but is happening because sometimes once() appears to be set to the Window which cannot be iterated with forEach().

I was able to reproduce on a clean install by adding a flowplayer player that uses their asnyc embed code into a Full HTML block, and adding it to a page with big_pipe enabled.

I then added the this logging to ajax.js:

    Drupal.ajax.bindAjaxLinks = function (element) {
    const els = once('ajax', '.use-ajax', element);
    
    if (!Array.isArray(els)) {
      console.log('ajax ded');
      console.log(once)
    } else {
      console.log('ajax works')
      console.log(once)
    }

When it works correctly, the log line looks like function augmentedOnce(id, selector, context) and clicking on it takes me to /core/misc/jquery.once.bc.js?v=9.3.0 as expected. When it's broken, the log line looks like function once(e, n), and clicking on it takes me to https://cdn.flowplayer.com/releases/native/stable/plugins/chromecast.min.js added by the player.

I think the issue is that web/core/misc/jquery.once.bc.js is attaching once() to the window object, and callers are making the assumption that when they call once(), the window.once method defined by that file will be the one that is called. I suspect that if something else happens to override window.once then the overridden version is called instead. I'm not sure what else would cause console.log(once) to change like that. I'd imagine that a solution to this would involve drupal's own once not being attached to the window and invoked globally, but instead attached somewhere where it will reliably be not overwritten.

Unfortunately I'm not sure of a good way to to reproduce this without the flowplayer embed, although I'd imagine it must be possible. The demo embed does not load up if you implement it yourself, so we might need to figure out some other way to get a player to use for testing. I cannot provide embeds that we are using because the content belongs to my client. If needed, I could set up a trial account and provide an embed to use, although they're only free for 14 days.

Re: #11 and #12

I can't reproduce it. Use a module or only HTML code?

EDIT:
above flowplayer example is outdated I think. Use below one, the player is shown but no errors

As I said, "The demo embed does not load up if you implement it yourself, so we might need to figure out some other way to get a player to use for testing." My link is not outdated, but the snippet on their examples page does not cause the error.

No additional modules. This was a "standard" installation of drupal with big_pipe enabled, and I added the embed via "source" of the Full HTML textarea in a custom block. Here is a version of our embed with IDs removed, the format outlined by https://flowplayer.com/developers/player/player-api#creating-a-flowplaye......

        <div id="async_flowplayer" data-player-id="our-player-id">
                      <script src="//cdn.flowplayer.com/players/our-player-id/native/flowplayer.async.js">
                        {
                          "src": "our-video-id",
                          "autoplay": "true",
                          "loop": "true",
                          "hls": {
                            "startLevel": "3"
                          },
                          "autopause": "true"
                        }
                      </script>
        </div>

I'll note that the player has the chromecast plugin enabled in our account backend, which I think is adding the chromecast script I noted from the logging.

#13 though seems to confirm that the problem is likely related to window.once being overridden and the subsequently invoked by Drupal.

Actually, #13 is great -- makes this easily reproducible. Just update core/once in core.libraries.yml to the following and navigate to the blocks page at /admin/structure/block.

once:
  remote: https://git.drupalcode.org/project/once
  version: "1.0.1"
  license:
    name: GNU-GPL-2.0-or-later
    url: https://git.drupalcode.org/project/once/-/raw/v1.0.1/LICENSE.md
    gpl-compatible: true
  js:
    assets/vendor/once/once.min.js: { weight: -19, minified: true }
    https://unpkg.com/alpinejs@3.x.x/dist/module.cjs.js: { attributes: { defer: true }, type: external, process: false }
  dependencies:
    - core/drupal.element.matches

Errors are present and the logging I mentioned in #10 above points to alpine's modue.cjs.js once method instead of Drupal's.

so it's a conflict with the global name once?

Well, the conflict is when both core and some other third-party define a global once, and core tries to call it after it's been modified by the third-party. Core can do something about this, because the break was introduced by core's addition of the new core/once and core/jquery.once.bc libs. Potentially just the latter, actually, but I'm not 100% sure. Before 9.3, my flowplayer embed and presumably alpinejs both worked fine, but now they do not.

jquery.once.bc.js does this, which is at least part of the issue.

  window.once = Object.assign(augmentedOnce, drupalOnce, {
    remove: remove
  });

It seems like the simplest thing would be to instead attach core's new once() method to the Drupal global or something like that (like behaviors already are) where it is safe from modification instead of adding it right on the window. There are potentially a significant number of third-party libraries out there that will now cause core's own js logic to totally break just by being loaded by the page.

Here's a simple proof of concept patch that does a find and replace from once( to Drupal.once (with the space present to avoid targeting PHP and only hit the JS once calls), and also changes window.once to Drupal.once.

For reproducable testing, simply update your drupal/once library in core.libraries.yml to the following (to include the "breaking" chromecast script I was encountering, without any alpinejs overhead), enable big_pipe, and visit the blocks admin page:

once:
  remote: https://git.drupalcode.org/project/once
  version: "1.0.1"
  license:
    name: GNU-GPL-2.0-or-later
    url: https://git.drupalcode.org/project/once/-/raw/v1.0.1/LICENSE.md
    gpl-compatible: true
  js:
    assets/vendor/once/once.min.js: { weight: -19, minified: true }
    https://cdn.flowplayer.com/releases/native/stable/plugins/chromecast.min.js: { attributes: { defer: true }, type: external, process: false }
  dependencies:
    - core/drupal.element.matches

Before the patch, you should see the once(...).foreach errors, and afterward you should not. Marking as "needs review" to call our friendly neighborhood testbot, but there may be more that needs to be fixed as well.

Maybe somebody more familiar with the test runner knows what "custom commands failed" means?

Ah okay, that's what I thought but wasn't sure. Didn't realize prettier needed to be run before tests. For anybody's reference, instructions at https://www.drupal.org/node/2986680. I think this one should be better.

EDIT: D'oh, this also replaced function once in the once.js. Will probably need to fix that but can't do that right now.

The once library is not tied to Drupal, it's a package you can use it wherever, see https://www.drupal.org/project/once. Changing assets/vendor/once files is not something we want to do

Understood, and in fact I did not mean to patch the once library itself. I'll be creating a new patch that does not touch its files. That said, my understanding is as follows (and if I'm misunderstanding any of the internals, please let me know):

The once package itself may be totally fine. In standalone projects you are able to namespace your imports and potentially avoid what we are seeing here. However, the way that Drupal leverages the package is problematic in that we have not namespaced it, instead deciding to attach a commonly used function name to the global namespace in jquery.once.bc.js. As long as calling once() in core/misc/ajax.js (or anywhere else) invokes window.once, any other library that also sets window.once is very likely to break Drupal's logic through no fault of the end user, and indeed there is no way to work around that.

Flowplayer and alpinejs should probably not be using the window object but, in the end, we probably should not be either. End-users could be pulling in any number of third-party libs and it seems odd that we'd be okay with breaking them when we actually don't need to.

Contrib started updating their code to use the new once function, it's going to be a problem to ask them to change it a second time, partially updated sites could be in trouble with this

Looking ahead, I agree with @ericpugh. There may be some short-term pain with asking contrib to make the updates a second time, but this seems to just have been an avoidable oversight with a potentially simple fix.

Further, existing sites that were working without issues (and theoretically should continue working, since this update was not intended to brick them) are already in trouble without a way forward. This feels potentially more problematic to me.

Alright, I created an MR for an updated version of the changes from #24 which:

• Does not affect any files in core/assets, including the once package itself per #25
• Also replaces instances of $(once(...)) that were missed by my initial find and replace
• Ensures that Drupal is actually passed into the closure in jquery.once.bc.js. I think this was the cause of the one functional test failure in #24

Reverting issue title -- not sure what happened. Also removed "draft" from the MR title because I think it blocked tests?

Updating the issue description to better reflect my current understanding of what's going on.

Needs work per failures: https://dispatcher.drupalci.org/job/drupal_patches/106604/console

There are some "Drupal.once is not a function" errors, so I'd assume that the Drupal global needs to passed in somehow for those cases. Looks like other tests just didn't get their once() calls replaced by the find and replace, so it makes sense that those are failing. I don't know whether those would also need to have the Drupal object made available for this approach.

I think I'm going to leave as-is for now until an approach is agreed upon since it seems silly to dial in tests for a solution that may not be used.

Sorry to spam this issue, but I think the once module itself is also to blame. I didn't realize that the minified version is just straight up setting var once = .... I'd assume this decision was made for ease of use, but we are seeing the side-effects here. This and the use of window.once both seem problematic to me.

@droplet I'm not sure how the conflict comes about from those 2 scripts, exactly, but adding them to a library as described does cause a conflict with core. Whether they are assigning to window.once or just setting a global var once, or whatever, I'm not sure. And I can't really speak to why the "old" version of the flowplayer script is being loaded on our site, but all I can say is that it is the one that they are serving to us with the embed.

I agree the global vars conflict still can be a problem with other scripts..The question is how common the `window.once` is.

I guess my thought is just that we should avoid polluting the global namespace if possible, regardless of how common a function name is. Especially if we want to start splitting more functionality into individual @drupal modules, we are going to open ourselves up to additional conflicts with each new module (as @ericpugh mentioned). @drupal modules could easily do something like the following and avoid the issue entirely:

var Drupal = Drupal || {};
Drupal.once = function(...) {...}

This would maintain the "iife" pattern and IE11 compatibility afaik, and adding methods, etc. to the Drupal global is already a well-established pattern.

Actually, I think you could implement the above with minimal headache for "contrib that has already begun upgrading". You could include both the "old" and the "new" versions of the scripts in core/once lib for the time-being. The global and Drupal scoped versions shouldn't conflict with each other, so I think that would allow contrib's global implementations to keep working while allowing core to instead implement Drupal.once. Global once() could then be given its own deprecation schedule, be that 10.0.0 or otherwise.

About alpine not sure what to do. Can't replicate, using the cdn.js version doesn't trigger any errors and can't load the cjs version without some more files around that makes loading commonjs in the browser works.

The following still reproduces the error (without the patch) on a standard drupal install from the blocks admin page, so I'd give it a try for your testing:

once:
  remote: https://git.drupalcode.org/project/once
  version: "1.0.1"
  license:
    name: GNU-GPL-2.0-or-later
    url: https://git.drupalcode.org/project/once/-/raw/v1.0.1/LICENSE.md
    gpl-compatible: true
  js:
    assets/vendor/once/once.min.js: { weight: -19, minified: true }
    https://unpkg.com/alpinejs@3.x.x/dist/module.cjs.js: { attributes: { defer: true }, type: external, process: false }
  dependencies:
    - core/drupal.element.matches

There are some other errors related to alpine, but both toolbar and bigpipe are throwing the once error. The other alpine errors are really irrelevant as long as the once errors shows up, since that's what demonstrates the conflict.

This works for the flowplayer use-case.

Sorry to say that it does not. I'm seeing less errors with the patch applied, but /admin/structure/blocks is still showing instances of the error from bigpipe.js line 50 with the patch applied. Seeing the same with the alpine script + patch. This is on a fresh, standard install.

For the flowplayer I'm just sad about the conflict, in both case I don't see anything simpler than changing the namespace.

Yeah I feel ya. I totally understand why it's all set up how it is. It's just unfortunate.

If we put stuff in Drupal, does it mean we'll want to do that for other libs? jquery, backbone, underscore, etc.? if that's the case might want to have a Drupal.utils namespace or something to avoid adding everything to Drupal.

Good point. Since jquery, etc, are already used as globals across the js ecosystem it may be alright to leave them there. But I can the see argument for consistency that if we put one lib in Drupal maybe they should all go there. I don't know that I'm familiar enough with all of the tooling to make a real argument one way or the other.

Fwiw, core already commonly does like Drupal.ajax, so it's not the end of the world to drop them right on Drupal, but if this is going to become a common approach then you're right that it might be a good idea to add something like Drupal.utils for the sake of organization.

Just want to drop a note in here that it'd be awesome to get a fix into 9.4.0 for this if possible since affected sites are stuck on 9.2.x until we figure out a fix, and 9.2 support is dropped with the release of 9.4.

@smustgrave issue summary needn't be updated because no actual decisions have been made regarding how this should be addressed, unfortunately.