Overview

ComponentSourceInterface::getClientSideInfo() returns an array shape with various bits of metadata required for the component listing in the React client:

  • Component ID and label
  • Default HTML markup
  • CSS and JS required to render the component

Is an array OK here or should we use a proper value object? Or can we even refactor this away entirely and just ask the component source plugin to render the component to a build array, and handle everything in the caller?

Proposed resolution

  • ApiComponentsController is essentially normalizing a list of Component config entities
  • … and hence the way to bring consistency to all this is to refactor away ApiComponentsController, by updating Component to implement XbHttpApiEligibleConfigEntityInterface just like Pattern does
  • … plus probably moving ApiConfigControllers::normalizePattern() to a new Pattern::normalize() (on the config entity class itself!)
  • … and similarly add a new Component::normalize() to contain most of ApiComponentsController

That would then also open the door to adding a new ClientSideRepresentation value object, which should accept a render array, which would take care of automatically generating default_markup, css, js_header and js_footer.

User interface changes

None.

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:

Comments

longwave created an issue. See original summary.

longwave’s picture

@larowlan also noted that this method handles cache tags specially, why only cache tags and not contexts or max-age? If possible we should remove this flag entirely here.

longwave’s picture

Assigned: Unassigned » longwave
Status: Active » Needs work

getClientSideInfo() now returns a build array where the HTML, CSS and JS can be extracted from. I also removed the cache tags flag as I haven't seen any problems with it locally.

We still need somewhere to get/put the props, slot data, and dynamic field candidates, but maybe those should become individual methods on the source plugin.

wim leers’s picture

Status: Needs work » Postponed (maintainer needs more info)

Is an array OK here or should we use a proper value object?

Thanks to #3482529: Refactor XB internals to not assume /xb-components HTTP API returns only SDC-powered XB Components, this is now a well-defined array shape.

Refactoring to a value object is fine, but what is the benefit? (Other than the well-defined array shape currently only being enforced using PHPStan, but that could easily be changed.)

Or can we even refactor this away entirely and just ask the component source plugin to render the component to a build array, and handle everything in the caller?

It's the client side that needs all the separate pieces of information; a "build array" (🤔 aka render array, right?) hence does not get us any further AFAIK.

See ApiComponentsController.

wim leers’s picture

longwave’s picture

Well, if the source plugin returns just a render array, then ApiComponentsController can be responsible for rendering it and extracting the CSS/JS, no? See https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

wim leers’s picture

Status: Postponed (maintainer needs more info) » Active

Of course! 😅

Agreed that would be nicer! 👍

longwave’s picture

Assigned: longwave » wim leers
Status: Active » Needs work

Locally the max-age is 1 instead of 3600 now. The original max-age is now bubbling up to the actual response even though we try to override it in the controller, but I don't see where the logic has changed for this to happen. Assigning to Wim in the hope that he has ideas or better renderer debugging skills than I do.

wim leers’s picture

Challenge accepted 🤓

wim leers’s picture

I lost track of this. This is still relevant and worth doing.

wim leers’s picture

#3491021: Leverage HTML comment annotations, remove wrapping div elements is touching adjacent areas and resurfaced the need for this clean-up.

Merging in origin/0.x, with ~1.5 month worth of changes 🫣

wim leers’s picture

In #3486203: Integrate saving sections with the backend: (de)normalize Pattern config entities using the client-side data model, I asked @f.mazeikis to add a @todo pointing here, because here is an additional occurrence of this pattern.

That makes this issue more important.


#3486203 landed last night, so pushing this forward now :)

wim leers’s picture

wim leers’s picture

Alright, now we're mostly back to the state @longwave left this at 2 months ago at #9, where he assigned it to me.

Where are we?

@longwave's MR got us to the point where ComponentSourceInterface::getClientSideInfo() implementations are pretty much pointless, because \Drupal\experience_builder\Controller\ApiComponentsController::getCacheableClientSideInfo() now does centrally what each implementation duplicated 👍

What's next?

  1. The sole failing test is PropSourceEndPointTest, which is failing in the way Dave described:
    There was 1 failure:
    1) PropSourceEndpointTest::test
    Behat\Mink\Exception\ExpectationException: Current response header "X-Drupal-Cache-Max-Age" is "1", but "3600" expected.
    
  2. #3486203: Integrate saving sections with the backend: (de)normalize Pattern config entities using the client-side data model introduced \Drupal\experience_builder\Controller\ApiConfigControllers::normalizePattern(), which pretty much duplicates what \Drupal\experience_builder\Controller\ApiComponentsController::getCacheableClientSideInfo() does.
wim leers’s picture

Issue tags: +D8 cacheability

The root cause for #9/#15.1 can be found by putting a conditional breakpoint on \Drupal\Core\Cache\Cache::mergeMaxAges(), with this condition: !empty($max_ages) && min($max_ages) === 1.

Doing so reveals that this occurs when the views_block:content_recent-block_1 block is being rendered; it is being caught by EarlyRenderingControllerWrapperSubscriber.

But why? 🤔 Debugging…

wim leers’s picture

Note that #9 is only (easily) reproducible in the test. This is an important hint!

Turns out that Views' rendering pipeline leaks cacheability when simply calling ViewsBlock::build():

(I bet there's a core issue for this somewhere …

buildRenderable

Put a conditional breakpoint on

          if ($renderer->hasRenderContext()) {
            $renderer->render($data);
          }

with the condition $data['#cache_properties'] == ['title', 'changed'], and you'll see this before the ::render() call:

array(4) {
  ["keys"]=>
  array(5) {
    [0]=>
    string(5) "views"
    [1]=>
    string(6) "fields"
    [2]=>
    string(14) "content_recent"
    [3]=>
    string(7) "block_1"
    [4]=>
    string(64) "86fc8fe8b57f924155e998c4788f7e8badf0989a44a5ae755a20ee7d6d15ec99"
  }
  ["tags"]=>
  array(3) {
    [0]=>
    string(6) "node:1"
    [1]=>
    string(6) "user:0"
    [2]=>
    string(32) "config:views.view.content_recent"
  }
  ["max-age"]=>
  int(-1)
  ["contexts"]=>
  array(0) {
  }
}

and afterwards, max-age has changed to 1. We're getting close!

Then I noticed that in the rendered result it said 55 seconds ago. That is obviously time-sensitive. That's the "timestamp ago" formatter.

So I put a breakpoint in \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter::formatTimestamp(), right where it has this line:

CacheableMetadata::createFromObject($result)->applyTo($build);

… and sure enough: that's where it's coming from! And now it makes sense why it doesn't (tend) to happen in a dev environment, and only in tests: because in a test, all content is freshly created.

🚨 But why was this not happening previously? Investigating now.

wim leers’s picture

Why this was not happening previously: the

$component->getComponentSource()->getClientSideInfo($component);

call was wrapped in ::executeInRenderContext(), and hence the ViewsBlock::build() was bubbling cacheability inside the render context that ::getCacheableClientSideInfo() was constructing.

The fix then is simply to continue to do that (but add a comment to avoid another person having to spend >1 hour debugging this): https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

wim leers’s picture

Assigned: wim leers » longwave
Status: Needs work » Needs review

The current MR is green, and has 7 files, +119, −79 as its diffstat. But 50 of the additions are just for updating test expectations because now there's less magic! So, really, this is a net reduction in LoC!

Next

Next up: #15.2!

But \Drupal\experience_builder\Controller\ApiConfigControllers::normalizePattern() is not dealing with Components, but with Patterns. So while it has to generate the same key-value pairs (default_markup, css, js_header and js_footer), none of the code is comparable.

So actually, there is nothing to do in that code in this issue.

However, it made me realize that:

  • ApiComponentsController is essentially normalizing a list of Component config entities
  • … and hence the way to bring consistency to all this is to refactor away ApiComponentsController, by updating Component to implement XbHttpApiEligibleConfigEntityInterface just like Pattern does
  • … plus probably moving ApiConfigControllers::normalizePattern() to a new Pattern::normalize() (on the config entity class itself!)
  • … and similarly add a new Component::normalize() to contain most of ApiComponentsController

That would then also open the door to adding a new ClientSideRepresentation value object, which should accept a render array, which would take care of automatically generating default_markup, css, js_header and js_footer.

So, I proposed an "alt" MR, that builds on top of the MR @longwave started: https://git.drupalcode.org/project/experience_builder/-/merge_requests/547

I'm totally fine with just landing the original MR right now and deferring the rest to a follow-up 😊 But @larowlan seemed to feel strong about one particular detail (despite the net LoC reduction mentioned above), so I'm also fine with doing this here, especially because I've proven it's totally within reach.

Goals from the issue summary

Or can we even refactor this away entirely and just ask the component source plugin to render the component to a build array, and handle everything in the caller?

We can't yet, because SingleDirectoryComponent::getClientSideInfo() is returning too many other key-value pairs. Getting all of those addressed here would make the scope too big.

Is an array OK here or should we use a proper value object?

I think it's okay as an interim step, but if you don't, then see the "alt" MR from above.

longwave changed the visibility of the branch 3484678-improve-or-remove to hidden.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

LGTM, I like the additional changes in MR!547 - RTBC once the nits from the review are done.

wim leers’s picture

Title: Improve or remove ComponentSourceInterface::getClientSideInfo() » Simplify ComponentSourceInterface::getClientSideInfo(), introduce ClientSideRepresentation value object, and leverage it
Assigned: longwave » Unassigned
Issue summary: View changes
Issue tags: +Performance

YAY, many thanks, @longwave!

This:

We should still refactor PropSourceEndpointTest into XbConfigEntityHttpApiTest::testComponent(), but there's so much cruft in there anyway that's going to have to change for #3455629: [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model fairly soon, that I think "just" this MR is progress enough.

If your git archeology lands you on this issue, please check the ℹ️ … comments I left on the MR at https://git.drupalcode.org/project/experience_builder/-/merge_requests/547, in which I tried to pre-emptively answer all reviewer Qs and tricky bits.

  • wim leers committed db607ea9 on 0.x
    Issue #3484678 by wim leers, longwave: Simplify ComponentSourceInterface...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed
lauriii’s picture

I'm getting following error after this issue:

AssertionError: assert(!array_key_exists('default_markup', $this->values)) in assert() (line 29 of /var/www/html/drupal/modules/custom/experience_builder/src/ClientSideRepresentation.php).

It doesn't seem that \Drupal\experience_builder\Entity\Component::normalizeForClientSide is providing the required values 🤔

wim leers’s picture

lauriii’s picture

Thank you @wim leers! 🙏

wim leers’s picture