Problem/Motivation

First, you need to know how BigPipe works at a high technical level:

BigPipe sends the page skeleton with the non-personalized parts of the page first. Let's call it The Skeleton. The personalized parts of the page are represented by BigPipe Placeholders that are replaced later.

The Skeleton of course can also have attachments. Including asset libraries. And those we track in drupalSettings.ajaxPageState.libraries — so that when we load new content through AJAX, we don't load the same asset libraries again. A HTML page can have multiple AJAX responses, each of which should take into account the combined AJAX page state of the HTML document and all preceding AJAX responses.

BigPipe does not use of multiple AJAX requests/responses. It uses a single HTML response. But it is a more long-lived one: The Skeleton is sent first, the closing </body> tag is not yet sent, and the connection is kept open. Whenever another BigPipe Placeholder is rendered, Drupal sends that by sending (and thus appending to the already sent HTML) something like <script type="application/json">[{"command":"settings","settings":{…}}, {"command":…}.

So, for every BigPipe placeholder, we send such a <script type="application/json"> tag. And the contents of that tag is exactly like an AJAX response. The BigPipe module has some JS that listens for these and applies them. Let's called it an Embedded AJAX Response (since it is embedded in the HTML response). Now for the interesting bit: each of those Embedded AJAX Responses must also take into account the combined AJAX page state of the HTML document and all preceding Embedded AJAX responses.

Given that, it should be clear that dealing with AJAX page state for BigPipe means tracking the libraries that are already loaded by either The Skeleton or one of the preceding Embedded AJAX Responses.

The problem so far was that is was effectively impossible to get the final AJAX page state for a response. The BigPipe patches until now used the hook_js_settings_alter() hack that mostly works, but doesn't work completely (because that hook is invoked for themes after it is invoked for modules, so theme modifications would always be lost).
Not to mention that it was extremely difficult to understand.

Proposed resolution

This patch fixes that, by:

  1. Requiring AssetResolver::getJsAssets() (which gathers the final JavaScript settings) by updating the AttachedAssetsInterface object it receives. Since that is the code calling hook_js_settings_build() and hook_js_settings_alter(), it also is the natural place for this.
  2. Requiring AttachmentsResponseProcessorInterface implementations to update the Response they are given to update that response's attachments to match the final values for those attachments.
  3. 1 + 2 = we no longer need hacks to track AJAX page state in BigPipe's logic.

Remaining tasks

Review.

User interface changes

None.

API changes

No API changes, only behavior changes:

  1. Behavior change: AssetResolver::getJsAssets() updates the AttachedAssetsInterface object it receives with the final settings (i.e. those gathered by getJsAssets() invoking hook_js_settings_build() and hook_js_settings_alter()).
  2. Behavior change: ResponseAttachmentsProcessorInterface implementations are required to update the Response's attachments to the final values (very similar to the previous point).

Data model changes

None.

CommentFileSizeAuthor
#2 2597359-2.patch9.62 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Active » Needs review
StatusFileSize
new9.62 KB
wim leers’s picture

Note this already got reviews in the original issue. Now it's just a matter of getting this in.

Copy/pasted those reviews for ease:

@Fabianx at #2469431-147: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts:

#141 is exactly what I originally envisioned that responses and new page state are coupled! I just did not know how.

Great work!

@yched at #2469431-149: BigPipe for auth users: first send+render the cheap parts of the page, then the expensive parts:

Also, #141 looks super elegant indeed. Awesome :-).
fabianx’s picture

Status: Needs review » Reviewed & tested by the community

RTBC - Looks great to me.

Should we unset all data that has already been processed? (I saw two new unset there).

wim leers’s picture

Should we unset all data that has already been processed? (I saw two new unset there).

The two unset()s are for #attached[feed] and #attached[html_head_link], both of which are just aliases/shortcuts/syntactic sugar for #attached[html_head] plus #attached[http_header].
Hence it doesn't make sense to keep #attached[feed] and #attached[html_head_link] around as final values, because they are meaningless: they are processed into other things.

catch’s picture

Issue tags: -rc target triage +rc target

Discussed with @alexpott, @xjm and @effulgentsia.

This seems sensible to have a clean BigPipe implementations.#

There won't be an actual behaviour change anywhere here until BigPipe is running either in contrib or core, so documenting the expectation earlier seems good.

catch’s picture

Do we need a change record for the interface @return clarification?

wim leers’s picture

I'd be happy to write one if you deem it necessary. But given that the issue that introduced this in the first place (#2407195: Move attachment processing to services and per-type response subclasses) didn't even have a change record, that seems not very helpful. Only a handful of developers will also ever have to use this.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 6c8f58a on 8.0.x
    Issue #2597359 by Wim Leers: Require responses with attachments to...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.