Refactor BigPipe to use HTMX

It's green:)

All green is super - nice work! This is direction really simplifies the changes in big_pipe.js and that’s a real plus.

Because bigPipe as we have it re-uses existing javascript from the ajax system, to remove the ajax system we need to replace that.
@nod_ had this list in a Slack discussion:

We need the equivalent of those 3 commands to implement bigpipe

• use Drupal\Core\Ajax\MessageCommand;
• use Drupal\Core\Ajax\RedirectCommand;
• use Drupal\Core\Ajax\ReplaceCommand;
• Add_css/add_js and the settings but that’s mostly available already

I think we need to identify the pieces of this list that htmx is not going to do, and implement those in general scope. Then the items that htmx does just fine in an ajax context, we need to have as helper methods in big_pipe.js and leverage the htmx api where we can.

I'm going to take them in order.

MessageCommand
In the ajax context, I would have implimented this in HTMX using a header and a custom event handler that calls our Drupal.Message function. This is actually the example in the trigger headers documentation.

HX-Trigger: {"showMessage":{"level" : "info", "message" : "Here Is A Message"}}

One thing that Drupal.Message does client-side is determine the selector for the message container. If there is a way to do that server-side then this could also be implemented as an extra out-of-band swap on the response without any additional javascript.

We do need a helper method for this that we can use in big_pipe though.

RedirectCommand
HTMX has both the HX-Redirect and HX-Location response headers. We don't have a response so we need a function for that in bigPipe.

ReplaceCommand
We also just need the bigPipe use case for bigPipe. This is one of the core competencies of htmx in an ajax request context.

addCSS and addJS
In the ajax context using htmx, these feel like out of band swaps as well.

fathershawn → closed merge request !12255

fathershawn → changed the visibility of the branch 3526267-bigpipe-with-htmx to hidden.

Here's an MR into @nod_'s branch moving the bigPipe command JS into bigPipe scope. It looks like the test setup scripts don't like MR's into a feature branch. The tests generally pass locally - 2 failures involving noJS exceptions. Not sure if that's a false negative as all of these changes are in JS.

If this is an acceptable direction then I think the next level of refactoring is to create the needed bigPipe Command classes to replace the AjaxCommands and update BigPipe.php to work without the Ajax API classes.

I still need to deal with Messages and Redirects, but this approach is simpler and over a 10 request sample results in a mean time of Load of 178.6 ms compared to 185.4 ms for the existing 11.x branch.

nod_ → closed merge request !12230

I'll close my branch that replace commands execution code with a htmx-powered code. This issue is way too late for 11.2 so let's try to do it properly for 11.3 :)

`BigPipeRegressionTest::testMessages_2712935` is now passing

BigPipeRegressionTest is now completely passing.

All tests green!

If this approach is good we should open an issue to theme individual messages to provide a server side way for themes to specify message markup

Haven't reviewed the MR yet but there's a longstanding existing issue with AJAX messages here 🐛 AJAX MessageCommand markup and styling differs from Theme default Active where either a drupalSettings or template approach is being looked at to allow server-side rendering of the markup. BigPipe is currently the most common way that messages get added to the page by MesssagesCommand (since 10.3) which caught out a lot of themes that weren't theming js messages ( 🐛 10.3 upgrade now missing status-message theme sugestions Active ).

Thanks for the issue link in #22 @catch! I'll go there and chime in!

Please add credit for @jrockowitz and @richgerdes who collaborated on test refactoring at the DrupalNYC Contribution Day

nod_ → credited jrockowitz → .

nod_ → credited richgerdes → .

credit

I posted in 🐛 AJAX MessageCommand markup and styling differs from Theme default Active and 🐛 10.3 upgrade now missing status-message theme sugestions Active proposals that would simplify the work in this issue.

nod_ → reopened merge request !12230

nod_ → closed merge request !12230

I think the only unresolved feedback relates to message theming and swapping. I would propose that those are handled in the related issues on message theming.

I took a look at the current MR here, and also MR 12295. With the current MR, I'm concerned about the number of changes needed in the BigPipe PHP code, but also the need to recreate the message handling.
Looking at MR 12295 it looks much more isolated to just the handling of AJAX commands.

Is it an option to split this work into smaller, more-reviewable chunks? And if so could we start by re-opening12295 and getting that in? I think that reduces the amount of changes needed and increases our chances of moving forward here.

Thanks for working on this

fathershawn → changed the visibility of the branch htmx-swap-placeholders2 to hidden.

fathershawn → changed the visibility of the branch 3526267-big-pipe-scope to hidden.

@larowan I've updated the issue summary to give an overview of how we got to where we are and the scope of MR-12295, which is our current MR. This MR combines work that @nod_ and I each did.

\Drupal\big_pipe\Render\BigPipe and clientside code are strongly connected so it is difficult to tease this apart. However, there is strong test coverage for BigPipe and we went through iterations to pass the tests.

MR-12476 communicates to me that I did not understand the limited scope of this issue. I understood that our goal here was remove the dependency between BigPipe and the existing Ajax API. @larowan has stated that to be too much change and @nod_ proposes to leave the dependencies between BigPipe and the four Ajax classes.

My question then is what is the plan and time line to remove these dependencies?

My question then is what is the plan and time line to remove these dependencies?

I haven't been following this issue closely, but speaking generally: often in situations like this, remaining work gets split off into follow ups. Reducing scope in individual issues makes the diffs smaller, easier to review, and more likely to be committed.

@fathershawn you didn't misunderstand, I think I didn't explained it clearly enough.

My initial plan was quick and dirty: do the replacement of the ajax commands only so we can get that done quickly and move on. When you added your MR that went with a more HTMX way of doing it, it looked interesting: changing ajax commands for a meta header and raw HTML looked like a great improvement, and you were really involved in making it work so I though it'd be good to try it out. It started to get hard with asset handling and messages. To make it work in that new way the changes and compromises started to become too much for comfort. After chatting with larolwan and catch we figured the less user-disrupting way to go about it was to do the initial quick and dirty implementation. That way if something breaks, we know it comes from a mismatch between what we expect and how htmx does things, because it's the only thing we changed.

Bigpipe is not broken, if we change the data format, how the data is sent or processed we risk breaking it in ways that are not obvious. Check out the history on the big_pipe.js file, there has been many fixes after we moved to the mutation observer handling, it took many years to find the bugs and fix them. I don't want to add more work on this area when there are other things broken we could work on.

Dealing with how messages are handled is going to be easier and less stressful when the change is by itself, not blocking a big feature MR. I second @godotislate assessment the smaller the diff the more likely it is to get committed quickly.

Thanks @nod_ for the clear explanation. :)

I'm happy to have learned about the internals of our BigPipe implementation. It was also worthwhile to understand how to remove the dependencies on the PHP side and that if we are to move to an HTML based approach, we need a way of rendering individual messages.

+1 for removing the ajax dependency on the client-side. I'm going to update the issue summary with some of what you wrote and change the issue title to reflect the scope.

We should open an explicit follow-up to un-AJAXify the PHP side. I think 🐛 AJAX MessageCommand markup and styling differs from Theme default Active might cover the messages issue, but does that need an HTMX equivalent or should try to fix that for AJAX and then use the same technique for HTMX?

fathershawn → closed merge request !12295

Follow-up issue to "un-AJAXify the PHP side" created and linked in the sidebar

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.