Problem/Motivation

ajax framework part of #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS.

Focus on the add_js command part of the solution, to iron out details of how scripts are loaded and what capabilities are supported (insertion order in the DOM tree, attribute support, etc.)

Tests require a change in the ajax command execution pipeline which is out of scope for this issue. Tests will be added in the parent issue.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3228352

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:

About issue forks

Comments

nod_ created an issue. See original summary.

nod_ opened merge request !1065

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Active » Needs review

depends on #3228351: Add loadjs library

nod_’s picture

nod_
Primary language French
Location Lille
commented
Related issues: +#3228351: Add loadjs library
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Add an add_js ajax command that uses promises » [PP-1] Add an add_js ajax command that uses promises

This includes #3228351: Add loadjs library. Marking as blocked on that.

Review posted!

nod_’s picture

nod_
Primary language French
Location Lille
commented
Title: [PP-1] Add an add_js ajax command that uses promises » Add an add_js ajax command that uses promises
nod_’s picture

nod_
Primary language French
Location Lille
commented

Probably needs some tests to make sure the new add_js command works as expected.

Wim Leers credited droplet.

Wim Leers credited olli.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Needs work

Crediting @olli and @droplet, marking NW for the unaddressed pieces of feedback.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Would still be great to land this before 9.3.0 🤓

yogeshmpawar made their first commit to this issue’s fork.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Looks like we missed the 9.3 boat. 9.4? 🤞

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 9.3.x-dev » 9.4.x-dev

bartlangelaan made their first commit to this issue’s fork.

bartlangelaan’s picture

bartlangelaan
Primary language Dutch
Location Netherlands
commented
Status: Needs work » Needs review

I added addressed all comments, please check MR !1065.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

I got a patch from this merge request however it does not work when using the clientside_validation module.

Perhaps someone that understands this functionality could integrate this solution into the clientside_validation module.

Until then I suggest instead a solution over here:

#1988968-268: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Needs review » Reviewed & tested by the community

Yes this issue only add the ajax command and doesn't do anything with it.

Making use of it and being able to test is left to #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS.

RTBC because it looks good to me and can't introduce regressions since it's not used anywhere yet :)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#18++

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update

Why are we adding this logic here and adding tests for it in #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS? It seems strange to add API like this without any test coverage when it's initially introduced. At minimum, we need to update the issue summary with reasoning for the proposed approach.

nod_’s picture

nod_
Primary language French
Location Lille
commented

Promise based code can be confusing and hard to get right.

Here we have only the add js part, and there is no impact on existing sites because we Don't change the logic of command execution, that new execution pipeline also involves promises so splitting this makes it easier to understand/review

At least that's the theory.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

Back to RTBC per #21.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

If there are no tests, what are the expected steps for validating that what is being done here is right? There's also no issue summary which makes it even harder.

I'm not yet convinced that the two step process here makes it easier to understand or review 🤷‍♂️

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue summary: View changes
Issue tags: -Needs issue summary update

The main issue was split in 3 so that we could get a quick commits for each of them individually. That didn't happen though.

We added loadjs #3228351: Add loadjs library but we have no tests or code using it, next we add this piece of code without building a whole thing just for testing this part, and the last commit changes the ajax command execution pipeline and integrates everything together with all the required tests and changes in the various modules needing it.

We could test it but it would take a lot of efforts when the tests can be done much more easily in #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS (and we would have to undo everything we added here).

nod_’s picture

nod_
Primary language French
Location Lille
commented

We had a few discussions on the piece of code I'm personally happy it wasn't in a huge patch.

we're used to monster patches but it doesn't have to be this way :) things can be split up.

catch’s picture

catch
he/him
Primary language English
commented

I think it's OK to add the new API here and add the test coverage for it in #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS, the other option would be merging the issues back together again, but that makes #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS bigger and more complex to review.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Priority: Normal » Major
Issue tags: +blocker

+1.

Perhaps what would put @lauriii's mind at ease is if we marked AddJsCommand @internal here and un-marked it in #1988968: Drupal.ajax does not guarantee that "add new JS file to page" commands have finished before calling said JS? 🤔

Also, #1988968 is major, has been around for more than a decade, and this blocks it. Updating issue metadata accordingly.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

ruiadr’s picture

ruiadr commented

Hello, this patch does not work as well.
In my case, I use s3 and store js & css on my bucket. When I am logged as administrator, for example, I can not use the form in the modal media.
To achieve this, I think you could replace PrependCommand and AppendCommand by AddJsCommand on this file AjaxResponseAttachmentsProcessor.php, like the first lines on this patch: https://www.drupal.org/files/issues/2022-01-25/1988968-268.patch

Without that, AddJsCommand is never used anywhere !

After this change, AJAX works like a charm. What do you think about that ?

megha_kundar’s picture

megha_kundar commented

HI,
i see pr merged for branch 9.3.x. But not seeing changes in 9.4.x.
Can anyone confirm if these commits are present for 9.4.x branch as well ?

bartlangelaan’s picture

bartlangelaan
Primary language Dutch
Location Netherlands
commented

RE #30

The MR is not merged yet, not for 9.3.x and not for 9.4.x.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Closed (duplicate)

Merging the two patch by committer request to make it possible to review. Credits have been added to the other issue.