Problem/Motivation
We've noticed that since updading to 9.5.0, there is some odd behaviour on some of our sites when logged in:
In particular, Drupal Behaviors were being applied twice (despite correctly implementing `context` which was working well before the upgrade). Further digging suggests that https://www.drupal.org/project/drupal/issues/3294720 introduced this as a fix for something else.
Drupal.attachBehaviors is called once, as expected, passing in `document` as the context.
It is then called a second time by big_pipe.js this time with `body` as the context.
This second call understandably catches other Behaviors and their attach methods are run again, with the event listeners being added another time (e.g. a menu toggle).
So far this can be worked around using `.once` but this doesn't feel like the best solution as my understanding is that's what Behaviors are for.
Steps to reproduce
- Install a fresh copy of Drupal 9.5.x (reproduced with 9.5.2)
- Go to a page that implements Big Pipe (for example, any page when logged in as administrator)
- Add a JS breakpoint inside the Drupal.attachBehaviors method declaration
-
Expected: debugger pauses with `document`, and future AJAX update contexts
Actual: debugger pauses first `document` as context; next it pauses with `body` as context
Verified by removing the lines added in https://git.drupalcode.org/project/drupal/-/commit/0c57711#80205155285d2... – functionality works as expected in this case.
I saw there are a couple of other issue related to this problem, but they are specific to a particular instance whereas this feels to be a broader problem.
Note: This is fine on a page that's not using Big Pipe, for example a normal anonymous user visiting a content page.
These is the issue I think introduced it: https://www.drupal.org/project/drupal/issues/3294720
And these issues are related.
https://www.drupal.org/project/drupal/issues/3314762
https://www.drupal.org/project/gin/issues/3314515
Proposed resolution
Change how the Big Pipe attachBehaviors call is implemented to use `context` in a way that doesn't cause other behaviors to be re-run
Remaining tasks
Implement a fix, review, test, confirm.
User interface changes
None.
API changes
None.
Data model changes
None.
Release notes snippet
TBC?
Comments
Comment #2
cilefen commentedDid you test by removing the changes in #3294720: The attachBehaviors() for document is only called after Big Pipe chunks are processed to verify that it in fact is the issue that changed this behavior?
Comment #3
psebborn commentedYes, I removed the changes from this commit https://git.drupalcode.org/project/drupal/-/commit/0c57711#80205155285d2... and the problem goes away.
I've updated the description to include this information, too.
Comment #4
abrammIn fact, this is an expected behavior.
Drupal behaviors may (and will) be called multiple times if something on a page changes, e.g. with either AJAX or Big Pipe.
Using once() is not a workaround; it's a requirement for any behavior in Drupal either initializing any objects specific to DOM element, adding event listener or any similar thing that should only be ran once.
It's being shipped with Drupal for the exact same purpose.
You may also notice once() allows passing in context as one of the arguments.
Comment #5
cilefen commentedComment #6
abrammComment #7
nyph1337 commentedI encountered this same issue on my project which previously had already had a lot of javascript implemented without using once(). This issue is genuinely the cause of turning off BigPipe, in fact at this point it is cheaper for our team to implement another custom mini-variation of BigPipe instead of reworking our behaviors to use once().
#4 This is absolutely an issue and your module shouldn't be designed to have it. While I agree that once() method gives you more guarantees and add more safety - it should be just a fail-safe for those rare cases when duplicate attachBehaviors() cannot be avoided. This should not mean that we all just start neglecting the context param and throw attachBehaviors() at will, uncontrollably. The context param should only contain the newly-added HTML that came through AJAX, and I am sure that BigPipe can and should be accountable of that, its just a bit more technically challenging.
The official drupal.org Javascript API overview docs never says its a "requirement" but rather a "guarantee" and the article is right to suggest it because a lot of modules, like BigPipe, do it wrong.
Comment #8
gpotter commentedI agree with #7, we have to make a conversion to make this work as well. It was a very easy to understand the concept before of attaching listeners to the context parameter (new html will flow through context once), but now being require to use once() everywhere adds additional complexity and boilerplate code.
Comment #9
abrammThe once() requirement is not something new; it's always being there and you could hit the same issue, for example, if you'd run AJAX on a page.
The concept of JS behaviors implies that behaviors could be ran multiple times on the same page, there's nothing new with it.
This may be easily confirmed be checking JavaScript API overview documentation page.
Comment #10
gpotter commentedSorry Abramm you are correct, just really running into these issues now at Drupal 9.5 after never having issues for years. I think many older tutorials make it vague and like its an either/or type of situation where attaching context to listeners is a magic bullet that is really not the case anymore (EX: https://www.lullabot.com/articles/understanding-javascript-behaviors-in-...)
Comment #11
fabianfiorotto commentedI already have trouble convincing front-end developers to use the context parameter in our custom modules (most of them just ignore it since don't understand its logic). Now I have to convince them to use jQuery once. When the rest of the javascript world is moving away from jQuery we are forcing people to use it. It doesn't make any sense.
Edit: I just realized that Drupal has its own version of once that is compatible with vanilla javascript but I still think we need a javascript API easier to understand for frontend developers that aren't specialized in Drupal.
Comment #12
larowlanFwiw once is a standalone library, not jQuery
Comment #13
dgtlmoon commentedhttps://www.drupal.org/project/drupal/issues/3347144 in this bug it fails to properly handle Drupal States (due to states binding twice when big-pipe is involved) when you have a fairly complex multi level states layout, so there is a bug here, I'm not sure if #3347144 is the correct fix or just fixing the symptom
Comment #14
abramm#3347144 is definitely a bug in States. It would fail with AJAX scenario as well, not only with Big Pipe.
Comment #15
silverham commentedEchoing sentiment :-(
All of our JavaScript/jQuery will have to be updated too.
From:
[my_theme.libraries.yml]
[main.js]
To:
[my_theme.libraries.yml]
[main.js]
Once API is documented here: https://www.npmjs.com/package/@drupal/once
[do not NPM install it, it is already in drupal core, ass add line via libraries.yml :-) ]