Change CodeComponentDataProvider::getRequiredXbDataLibraries() to base its logic on information provided by the front-end rather than on naive string/regex matching

I think what we'll want to do here is something similar to how we do first-party imports, where the XB front-end parses the JS to determine the dependencies and includes that information in the request payload when saving the component. We might need to add a new key to the js_component config schema to store the info if dependencies doesn't work for depending on Drupal libraries.

#2++

That's literally what I wrote at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

We'll need something similar for code components fetching data via HTTP (either local aka Drupal-served URLs or remote URLs), because that is what needed to in the future add server-side prefetching of data without breaking existing code components or leaving them behind.

All of this is basically to say: dependency information is crucial! Without dependency information, the backwards compatibility break risk assessment is "unavoidable, will be painful and require manual intervention", whereas otherwise it can be "avoidable, will either be transparent or precise instructions can be provided".

IOW: it's a matter of future DX for code component creators and update path feasibility for XB maintainers. See point 3 of my comment #12 in the issue that landed this.

Step 1 here is the front end providing that dependency information, because what #3531249: Populate data in `drupalSettings` to enable simple use cases in dynamic code components introduced (and what's quoted in the issue summary) is the best the back end can do, since it cannot parse JS.

Which is why I wrote

1. 👍 The current MR is based on the simple on/off presence of preg_match('/drupalSettings\.xbData/', $component->getJs(). I agree with the pragmatism until such a time we can use explicit import statements.

— #3531249-12: Populate data in `drupalSettings` to enable simple use cases in dynamic code components.1

So, my concern in one sentence: we must be able to provide a long-term update path for code components, which includes not breaking any code components created on beta1.

Implementation outline

My interpretation of what @effulgentsia and I are saying:

1. client-side to pass drupalSettings.xbData.v0 dependency information however it sees fit — note this COULD be all of the data initially, and just more granularity later! What matters is that we know which code components use this at all, and which version they're on. Which is exactly what the pragmatic approach in #3531249 (i.e. in HEAD did)
2. update \Drupal\experience_builder\Entity\JavaScriptComponent::updateFromClientSide() to consume it
3. remove the parsing logic in ::getRequiredXbDataLibraries() and replace it with a new config entity property on the JavaScriptComponent config entity type to store it instead
4. update the logic in ::getAssetLibraryDependencies() to use that new config entity property

IOW: the exact same pattern that #3522217: JsComponent-sourced rendered component instances lack cacheability of underlying config entities (+ first-party imports + auto-save tag when previewing) did, just not stored as an enforced config entity dependency.

Choice: beta blocker 🆚 stable blocker + update path by relying on heuristics

This means that this can indeed be a stable blocker, not a beta blocker, but only if the logic in getRequiredXbDataLibraries() can be updated to work with reasonable reliability, because then we can transform whatever that logic computes to explicit dependency information in an update path! 🎉

Per #3531249-18: Populate data in `drupalSettings` to enable simple use cases in dynamic code components:

The front-end API will consume this settings with methods (not decided yet) like getPageData() (for title and breadcrumb) and getSiteData() (for base URL and branding).

That's why I think detecting the presence of such calls in the JS is doable in a saved config entity, which means that an update path after beta1 would be feasible.

So: I propose to indeed keep this as a stable blocker, but I might have missed something 😇

Discussed with @balintbrews, he's on board 👍

+1 to this plan

Great!

So first #3534617: Build Breadcrumb, Site branding, and Page title code components plus the yet-to-be-created "page title code component" issues should land.

I provided context about this plan at #3534617-3: Build Breadcrumb, Site branding, and Page title code components.

That's become a single issue, so postponed on a single issue 👍

Expanding on

3. remove the parsing logic in ::getRequiredXbDataLibraries() and replace it with a new config entity property on the JavaScriptComponent config entity type to store it instead

What we need is dependency information in each code component:

data_dependencies:
  drupalSettings:
    - baseUrl
  urls:
    - /jsonapi/something
    - https://example.com/something-remote

Once we have that, we can have the server do prefetching, eliminating the flicker visible in #3530958: Build a navigation code component.

We discussed this at DDD Leuven. It just is not in scope for beta1 because so much needs to happen.

Looking at #12 I think some of this we should ask the component to export this information

For SSR we will likely need additional exports for components to return information like prefetched data and cache metadata

Should we consider adding an additional method to allow the component to return preloads? Parsing that from the AST is going to be fraught.

Imports we can parse with the AST, but things like urls inside fetch calls is going to be brittle.

export const getPreloadUrls = () => (['/some/url', '/some/json/api']);

The blocker is in, @effulgentsia asked me to look at this one.

Just realised there's an addDataFetch reducer and a third tab in the code editor 🤯

Neat. following the existing pattern I was able to add generic fetch support, how did I not know about that tab until now, amazing

So to achieve #12 I think we should probably split `dataFetches` into groups - one for uris and one for utils like getPageData
But this all seems doable. Have pushed my WIP and will continue on monday.

Unassigned in case someone else wants to pick this up in the meantime.

Continuing work here

Spent some time here trying to make use of the existing dataFetches property in the slice to populate the dataDependencies property on the backend.

The issue we have is one of timing.

We clear that property every time the source code changes in collectImportedJsComponents, and then as the code calls the methods, these entries are added back.

The issue is, at the time we update the autosave, we've just cleared the property and hence it is always empty.

To try and do this in a non-async fashion so we can get a definitive value without needing to wait for the iframe to post messages I imported @babel/traverse to attempt to gather this from the ast.

Some of these are easy, like detecting getSiteData and getPageData.

Where it gets tricky is finding and resolving the value of things we call fetch with - I can use traverse to find all CallExpressions where the callee.name is fetch. We can also look for the getResource call on the JSON API client. But finding the calls and resolving the value are two different beasts.

fetch('https://example.com') is easy to resolve because we have a string literal.

But fetch(url); is a bit harder because we have an identifier instead, and then have to resolve those too. e.g. url could be the result of a function... And it just keeps getting more and more complex.

I'll see if I can find a way to do something that doesn't require clearing the dataFetches values from collectImportedJsComponents but I'm wary of retaining values from the last preview re-render as they could obviously get out of sync as the component changes.

After a spike on @babel/traverse, I've gone back to reviewing the original goal of this issue, which is to remove the string matching in CodeComponentDataProvider::getRequiredXbDataLibraries

But this was expanded in #12 to also allow for identifying URL requirements so we can add <link rel="preload"/>. However this expansion of scope is what is causing the difficulties in #19

I think we should split off the preloading in #12 off into a separate issue as it is more involved.

I'm going to focus just on CodeComponentDataProvider::getRequiredXbDataLibraries here

Added #3538273: Use AST to identify resources fetched by CodeComponents so they can be preloaded/prefetched for part 3 of #12

Created draft change record and started on update path now we're beta stable

Opened a draft MR, still to go - e2e test

Looking at #12 I think some of this we should ask the component to export this information

+1

I think we should split off the preloading in #12 off into a separate issue as it is more involved.

WFM, but then that needs to become a beta blocker. Made it so in #3538273-3: Use AST to identify resources fetched by CodeComponents so they can be preloaded/prefetched.

This is ready for review.
I _think_ this is the first issue with an update path, because the change here to expect this in the config entity is a breaking one for old components.

I've added a db dump based on 0.7.2-alpha1 and core 11.2.2 that is minimal + the base recipe + the test_site recipe + some test modules that provide components, an update path and an update path test.

There is also a change record.

So great to have the first update path test coverage! 🎉 And thanks for the change record, too!

XB should aim to follow the best practices being proposed for core by @catch at #3521618: Add generic interface + base class for upgrade paths that require config changes.

Complication: the DB dump doesn't work on all DBs due to

      // Only the SQLite driver has this field map to due to a fatal error
      // error caused by this driver's schema on table introspection.
      // @todo Add support to all drivers in https://drupal.org/i/3343634
      'json:normal'     => 'JSON',

(you can see this in the PHPUnit CI jobs I queued for the non-SQLite DBs)

I don't want to block a commit on that, so will added explicit skips on those databases, and defer fixing that to a follow-up. The most important thing is the presence of an update path; it's only the test fixture that happens to have DB-specific needs.

Follow-up created: #3538537: Update path tests should also run against non-SQLite DBs.

Given this is the very first update path, I prioritized this issue, to start gaining experience for the post-1.0.0-beta1 reality, and to establish best practices.

So:

1. compiled with config entity update path best practices as prescribed by @catch's #3521618: Add generic interface + base class for upgrade paths that require config changes
2. to facilitate that, changed the default of dataDependencies in the PHP object to NULL
3. tightened dataDependencies to only specify key-value pairs it actually has data dependencies on, which required updating the client-side logic + test coverage
4. the client-side test coverage only tested the 2 extremes (zero data dependencies and ALL of them), nothing in between — that's now tested too 👍
5. added explicit validation test coverage for dataDependencies
6. thoroughly manually tested the code component editor UI
7. skipping the update path test on non-SQLite DBs, created #3538537: Update path tests should also run against non-SQLite DBs to transform the test fixture automatically when testing anything but SQLite

Finally: tried reverting the changes to an exported code component and sure enough:

1) /Users/wim.leers/core/modules/contrib/experience_builder/src/ExperienceBuilderConfigUpdater.php:88
JavaScriptComponent config entities without "dataDependencies" property is deprecated in experience_builder:1.0.0 and will be removed in experience_builder:1.0.0. See https://www.drupal.org/node/3538276

👆 That proves stale recipes/modules will now trigger deprecation errors! 🥳

CR published: https://www.drupal.org/node/3538276 🎉

See y'all in:

• #3538273: Use AST to identify resources fetched by CodeComponents so they can be preloaded/prefetched
• #3538537: Update path tests should also run against non-SQLite DBs

Changes look good to me, thanks