For BigPipe to be committed to core, we need the JS to be as clean, well-documented and as simple as possible.

I believe we can still improve the current code:

  1. the use of Drupal.Ajax, but that is being handled in #2670426: Improve BigPipe's use of ajax.js because it requires very thorough reviews
  2. very incomplete documentation in big_pipe.js
  3. the misleading concept of "container" in big_pipe.js, with only one mention of that on the server side
  4. the server side calls it "start and stop signals", the client side calls it "start and stop tags/elements"
  5. bigPipeProcessPlaceholder() receives placeholder but then uses this, which happens to contain the same value thanks to jQuery magic — we should just use the parameter we receive instead
  6. the data-drupal-ajax-processor="big_pipe" attribute is completely useless and actually more distracting/misleading than it is helpful
  7. last but not least: the asset library dependencies are wrong: big_pipe.js does not depend on core/drupal (because it doesn't use Drupal.behaviors), but it does depend on core/jquery.once, but that dependency is missing
CommentFileSizeAuthor
#2 2670462-2.patch5.06 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: wim leers » Unassigned
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2670462-2.patch5.06 KB

And patch.

fabianx’s picture

fabianx commented

It looks great to me and can be merged IMHO. Leaving for nod_ to RTBC, else this can just go in.

  • Wim Leers committed 8d5abaa on 8.x-1.x 
    Issue #2670462 by Wim Leers, Fabianx: Clean up, document...
wim leers’s picture

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

I've been pinging @nod_ for reviews of this for well over a week now, I'm assuming it won't happen (Drupal 8.1 cut-off is next week).

Since this is 100% code clean-up and 0% logic changes, I'm 99% certain he's okay with everything in this patch.

+++ b/js/big_pipe.js
@@ -8,23 +8,26 @@
+   * Executes Ajax commands in <script type="application/vnd.drupal-ajax"> tag.

Only changed this to application/json on commit, because that MIME type is being introduced in the BigPipe module at #2670444: Embedded AJAX responses aren't application/json, but application/vnd.drupal-ajax.

Status: Fixed » Closed (fixed)

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