In #3500042: Auto-save code components will allow Code Component that are edited in the UI to have an auto-saved state.
This means the edits to them should not affect the live site if they have already been placed. See also #3500043: Publishing code components
Since we are not using Workspaces in #3455753: Milestone 0.2.0: Early preview we can't do a real config entity save until we publish the changes in the Code Component because otherwise that would affect the live site.
As far as I know this is the only case where when are rendering Component inside XB (which itself is using an auto-save state) we need to consider that Component will have an auto-save state that should override the config. SDC and Block components don't have this dynamic.
⚠️ This assumes that the auto-saved code component is working. If it doesn't, then it'd be the client-side equivalent of #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log), which needs its own issue. Raised this at #3499919-20: [Meta] Plan for in-browser code components.
When rendering a Code Component inside XB it should use the auto save if there is one. When we render the code component in regular entity render it should not use the auto-save state
When thinking of a solution we should keep in the mind that after #3455753: Milestone 0.2.0: Early preview when we can use Workspace we might not need this functionality because we may be able to rely on a real config save if using Workspaces Extras module which has a sub-module Workspaces Config
\Drupal\experience_builder\ComponentSource\ComponentSourceInterface::renderComponent could check if the current route is experience_builder.api.preview and if it is rendering using the auto-save state
this might be we could encapsulate the logic in CodeComponent as this might be the only part of the system. It also might be easy to remove if we determine using Workspaces Config is better for this when it is available
class XbPreviewConfigOverride implements ConfigFactoryOverrideInterface {
/**
* {@inheritdoc}
*/
public function loadOverrides($names) {
if (!$this->routeMatch->getRouteObject()?->getOption('_xb_use_template_draft')) {
return;
}
$overrides = [];
foreach ($names as $config_name) {
// Check if $config_name is for a "code component", otherwise continue.
// Use \Drupal\experience_builder\AutoSave\AutoSaveManager::getAutoSaveData() to get that data.
// Assign to overrides.
}
return $overrides;
}
/**
* {@inheritdoc}
*/
public function getCacheSuffix() {
return 'XbPreview';
}
/**
* {@inheritdoc}
*/
public function createConfigObject($name, $collection = StorageInterface::DEFAULT_COLLECTION) {
return NULL;
}
/**
* {@inheritdoc}
*/
public function getCacheableMetadata($name) {
$metadata = new CacheableMetadata();
$metadata
->setCacheContexts(['route'])
->setCacheTags([AutoSaveManager::CACHE_TAG])
->setCacheMaxAge(0);
return $metadata;
}
}
| Comment | File | Size | Author |
|---|---|---|---|
| #36 | Screen recording 1080p.mov | 29.69 MB | f.mazeikis |
| #10 | xb-js-component-states.jpg | 174.77 KB | balintbrews |
Show commandsStart 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:
changes
Comments
Comment #2
tedbowComment #3
tedbowI bumped it to major only because I think we should decide sooner rather than later if this going to need changes to the system outside of Code Components. I made possible way we could do this that might just apply to the Code Components code
Comment #4
effulgentsia commented"Possible solution #1" seems good to me.
Comment #5
wim leersThat solution might stop working once #3491701: [later phase] ApiLayoutController must use the previewed route's controller, and override canonical content entity routes' received entity object happens. But we'll cross that bridge when we arrive there.
More short-term concern: no
ComponentSourceplugin should be aware of the global/request context, which this would violate. But as long as it's documented with an explicit caveat and bound to change prior to 1.0, then I'm fine with it as an interim step.Long-term, I think this ought to be implemented as
\Drupal\Core\Config\ConfigFactoryOverrideInterface. That's the mechanism that Drupal core provides for context-dependent overrides of stored configuration. Would that even be more work to implement? Because that'd set us up better for the future AFAICT. 😇Comment #6
tedbowAre we sure long term we will need this at all if rely on
wse_config?We have requirement out auto-save js components but this seem like more a UX requirement than a specification for how this would handled as far as backend storage. Could we just do real config saves, to back up work without user action and just let
wse_configbe responsible for making sure that doesn't pollute the live site? (relating #3493461: Identify roadblocks to staging config in Workspaces (e.g., via wse_config) to this issue for that reason)I think in #3475672: Research: Possible backend implementations of auto-save, drafts, and publishing the 2 main reasons we decided we needed some auto-save mechanism is that not a real entity save was
For 1 we could still use a simple auto-save if the validation does not pass but a real entity save if it does. In the case of saving content entities we are not using the auto-save state, which might not be valid, to then render the content entity outside of the XB UI itself or to render that entity in the XB UI for another entity besides itself. Would that be the case for the JS components? What would even happen if we tried to a render JS component config entity overridden by ConfigFactoryOverrideInterface in XB UI for another entity, say Page content entity, if the auto-save state is not valid?
For 2 do we have the same overhead concerns for JS Component config entities as we do for Content entities. If XB is going to be the default way some sites add new and edit existing Content and as well as JS Component, it still seems likely that sites will not be adding as many JS components as they would content entities.
Comment #7
wim leersYou're right: in principle not, because we'd just rely on
wse_config'sConfigFactoryOverrideInterface. But there's still much uncertainty AFAICT on all things "config in workspaces", so consider it just for the worst-case scenario, wherewse_configdoes not get us as far as we hope. (@traviscarden is working this quarter on writing the test coverage to help raise the confidence level on that front!)Is this referring to the description in the issue summary?
Everything else you wrote is actually closely related to what I just surfaced in our call, and which I captured at #3499931-6: HTTP API for code component config entities. I'll link to your excellent #6 from over there 👍
Comment #8
effulgentsia commentedI agree that the scope of this issue should not be to solve it for the long term; the long term is workspaces.
For the short term, I think either the ComponentSource plugin inspecting the request and loading from the autosave record instead of the config, or implementing a
ConfigFactoryOverrideInterfaceservice that inspects the request and returns overridden config so that the ComponentSource plugin can stay in its lane, is a great option. The latter is probably better in terms of purity, so if it's the same amount of work to do either, might as well do the nicer thing even if it's short lived. "Short lived" could still be a few months, depending on how long it takes us to do all the work needed to integrate Workspaces into XB. If the config override is a lot of extra work or introduces various other headaches, then it's probably not worth it, and adding a bit of ugly code into the ComponentSource plugin isn't so bad.Comment #9
effulgentsia commentedComment #10
balintbrewsAsked by Wim, I'm attaching the diagram we've been using to discuss this problem space.
Comment #11
wim leers⚠️ This assumes that the auto-saved code component is working. If it doesn't, then it'd be the client-side equivalent of #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log), which needs its own issue. Raised this at #3499919-20: [Meta] Plan for in-browser code components.
Comment #12
wim leers#6: FYI, the test coverage for
wse_confighas started over at #3505900: Add test coverage for staging simple config.#7: AFAICT
wse_configdoes not actually useConfigFactoryOverrideInterface: noconfig.factory.override-tagged service in https://git.drupalcode.org/project/wse/-/blob/2.0.x/modules/wse_config/w..., nor in https://git.drupalcode.org/project/wse/-/blob/2.0.x/modules/wse_config/s.... Looks like it was never discussed either: https://www.drupal.org/project/issues/wse?text=ConfigFactoryOverrideInte... and https://www.drupal.org/project/issues/wse?text=config.factory.override&s... both find zero results.That necessarily means it must do something much more heavy-handed instead.
Given my work almost a decade ago on
PirateDayCacheabilityMetadataConfigOverridefor #2524082: Config overrides should provide cacheability metadata, I'm 75% confident that what I added as option 2 to the issue summary will work fine. AFAICT we don't need\Drupal\Core\Config\ConfigFactoryOverrideBase.Comment #13
nagwani commentedComment #14
larowlanSounds like option 2 is the preferred approach, I'll start on that
Comment #16
larowlanImplemented option 2
Comment #17
tedbowThe tests are failing because the
experience_builder__autosavecache tag should be expected now in other testsComment #18
larowlanTests are passing now
I added a test to address @tedbow's comment - but it appears it hasn't gone far enough
Comment #19
wim leersI think this is ready, with two exceptions:
route.namecache context: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...::getProps()should guarantee to return anarray: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...AFAICT @tedbow's test concerns actually are already addressed: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6.... @tedbow, can you double-check? 🙏
Comment #20
wim leersThe sort of mind-bending aspect of config overrides and cacheability bubbling, that actually proves @tedbow's concerns are tested: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
Comment #21
tedbowglobal-regions.cy.js e2e test failed but this appears random and it passes locally for me
Comment #23
tedbow👹 is in 🎉
Comment #24
larowlanThought about this some more and we also need a way to have the Astro island load the draft JS and CSS
NW for that
Thanks for all the work overnight
Comment #25
wim leers#24: D'oh! YES! 🙈
Comment #26
larowlanRough plan to address #24
* Emit an event from auto save manager
* Subscribe to event in \Drupal\experience_builder\EventSubscriber\AssetGenerator
* Either new method in \Drupal\experience_builder\AssetManager for autosave _ OR _ have the component return a different asset path if in preview in \Drupal\experience_builder\Entity\XbAssetInterface::getCssPath and \Drupal\experience_builder\Entity\XbAssetInterface::getJsPath
Comment #27
effulgentsia commentedI don't think we want the autosave (aka preview aka draft) JS and CSS files written to disk. It would mean a bunch of short-lived files that we'd need to purge at some point. What if we define a route for those to serve them dynamically? We'd need the path to contain the ID of the component in that case. And I think we don't need the hash, since I think we'd want the preview to just show whatever is latest in autosave storage even if that got updated between the time that the preview HTML is sent and when the browser makes the request to the asset.
It would be great if the preview asset URL was
some/path/JS_COMPONENT_MACHINE_NAME. That would make a future issue with import maps nicer. For example, we'd be able to map@/components/tosome/path/and code in one component that doesimport Button from '@/components/button'would work without needing every component to be listed separately in the import map. We don't need to add anything import map related to the scope here, I'm just giving context for whysome/path/JS_COMPONENT_MACHINE_NAMEis preferable tosome/path/HASH.js, at least for the preview/autosave case. The current behavior ofHASH.jswritten to disk is still good for the live site (real config save) case.Comment #28
larowlanWorks for me 👍
Comment #29
larowlanComment #31
larowlanMR for using draft (autosave) version of the CSS/JS is up, implementing #27
Needed a fair bit of tinkering to get the astro island to support the #preview key, but that was why we added it originally.
Comment #33
f.mazeikis commentedComment #34
f.mazeikis commentedResolved merge conflict, tests passing and the code makes sense.
However, trying this out on local I see no draft CSS changes being applied to the actual preview. This seems to be regression, as few days ago at least saved CSS of the component would show in preview, but now neither saved nor draft CSS is being applied during preview. Looking into this.False alarm, all good.
Comment #35
wim leersfor #34 👍
In addition to that reason for changing the status: While working on #3505993: Code Components as Block Overrides, step 1, another oversight in addition to #24 became clear:
\Drupal\experience_builder\Config\XbConfigOverridesshould not only only apply to that route, but also only to users who have sufficient permissions: if they lack theadminister code componentspermission, they should not be able to see draft (auto-saved) states of edited code components.I think that can become a third MR here.
Comment #36
f.mazeikis commentedActually, found an issue, posted comment on MR.
TL;DR: Until Code Component is not saved with _something_ in it's CSS field, preview doesn't include draft CSS changes.
Attaching screen recording for clarity.
Comment #37
wim leers@f.mazeikis thought this MR (694) was ready to be merged, but I spotted several (small!) bugs: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6.... One is quite interesting: the use of
Cache-Control: privatemay result in the browser not actually using the latest auto-save draft, which would have been a pretty annoying front-end bug to track down! 😄Pushed an (untested!) commit for the trickiest bit I'm contesting, but leaving the rest for @f.mazeikis to address.
I think #35 can become a separate MR, that can land separately, after !694 is merged.
Comment #38
effulgentsia commentedI don't think that's correct. If they have permission to use the code component within XB's page builder (if we don't yet have granular permissions for this, this is currently the same as if they have permission to use XB's page builder at all), then they should see what their component instance looks like in their content, using the draft state of the JS and CSS.
Comment #39
wim leersI'm referring not a Content Creator, but to an anonymous end user. We can handle that in #3508694: Permissions for XB config entity types 👍
Comment #40
larowlanThe last open item here is how we communicate that a config entity is in preview state.
I originally had a flag on the config entity.
Wim pushed back on that and changed it to use cache max age of 0.
I don't agree with that change.
But I also see Wim's point about not putting it on the config entity because that communicates it is something you can set, when in fact its only a run-time flag.
I think we should look to how layout builder handles this in core - https://git.drupalcode.org/project/experience_builder/-/merge_requests/6...
tl;dr I think we should make buildRenderable have an explicit 'in preview' argument, and then use that to set a flag on the source plugin.
Then the source plugin can call take care of passing #preview TRUE along to the AstroIsland element.
This means we can also do this for the Block component source plugin and therefore respect the existing 'in preview' flag on block plugins and in the future layout plugins. I worked on the core issue to add this support for blocks and make use of it in client projects - it even filters through to block plugin templates.
I think that's a win-win.
I'll hold off implementing that until I get a +1 to that approach from Wim - assigning to him for a plus or minus one.
Comment #41
wim leers#40: 🏓 at https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... — basically: +1! 😄
Comment #42
wim leersAlso: massive blast from the past! 😄
Comment #43
larowlanIn relation to your interpretation
If we remove that, then the source plugin will be responsible for loading the config entity from storage _or_ from auto save.
But we'd also need to change how AstroIsland worked (it too loads the entity). One approach could be instead of passing an ID for component ID, we'd have to pass the entity.
But I think this will get us into issues with render caching (we don't want the entity to be serialized in a render array).
So maybe that highlights a further issue with the AstroIsland element. Perhaps it is too tightly bound to the JavascriptComponent config entity. Perhaps it doesn't need to load the component. Looking at the process method, it uses the component to
a) get the component url
b) get the component label
c) attach the component library
All of these could be done from JsComponent::buildRenderable instead. This makes AstroIsland less bound to JavascriptComponent and means it could possibly be used by another source plugin.
And it means we can ditch the config override.
I'll push ahead on that basis.
Comment #44
larowlanImplemented #43 and it turned out pretty nice I think.
Back to NR
Comment #45
wim leersComment #46
wim leersAfter a whole unexpected saga in https://git.drupalcode.org/project/experience_builder/-/merge_requests/6..., this is finally done!
Comment #48
wim leersComment #49
wim leersRegression: #3508922: Regression after #3500386: import map scope mismatch when previewed code component's JS is a 307 due to it not having an auto-save/draft — relates to #22.
Also, another thing we forgot: we made sure to generate a
experience_builder/asset_library.<name>.draftasset library … but we never actually load that 😅 IOW: we forgot to updateexperience_builder_page_attachments(). Created #3508937: Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI for that.Comment #50
nagwani commentedComment #52
wim leersAFAICT the test coverage this added was wrong: #3532414: Follow-up for #3500386: tighten `::collapse()` to improve data integrity, because new props added to auto-saved code components cannot have a widget anyway.