Overview
@balint.kleri and I determined the root cause for code components failing to render in the XB preview since #3500386: Code Components should render with their auto-saved state (if any) when rendered in the XB UI.
@larowlan, @effulgentsia and I decided last night that passing an $inPreview=TRUE flag should cause the auto-saved XB code components + asset libraries to be loaded.
But, if there’s no changes to the JS yet, then Drupal\experience_builder\Controller\ApiConfigAutoSaveControllers::getJs() returns a 307 pointing to the original, that causes the _import map scope to change from the draft/auto-saved component URL (component-url="/xb/api/autosaves/js/js_component/another") to the live/saved component URL (component-url="sites/default/files/astro-island/pKL_OwLDMlNxnBnBYIsn9BSu349KEpkBmq26qw_z8EQ.js").
And apparently this makes browsers not treat the originally requested URL as the scope, but the redirected URL.
Proposed resolution
This actually just points to a single innocent missing if-statement:
diff --git a/src/Plugin/ExperienceBuilder/ComponentSource/JsComponent.php b/src/Plugin/ExperienceBuilder/ComponentSource/JsComponent.php
index a81f561b0..8690a4ed3 100644
--- a/src/Plugin/ExperienceBuilder/ComponentSource/JsComponent.php
+++ b/src/Plugin/ExperienceBuilder/ComponentSource/JsComponent.php
@@ -119,6 +119,8 @@ final class JsComponent extends GeneratedFieldExplicitInputUxComponentSourceBase
\assert($autoSave->data !== NULL);
$component = $component->forAutoSavePreview($autoSave->data);
}
+ // Do NOT consider this a preview if there's no auto-saved draft version.
+ $isPreview = FALSE;
}
$access = $component->access('view', return_as_object: TRUE);
(Which will ensure that the JS URL that is used is only the auto-save one if an auto-save actually exists. Same for the component's CSS asset library.)
→ https://git.drupalcode.org/project/experience_builder/-/merge_requests/702
⚠️ Before arriving at that realization, I thought that the auto-save version of the code component was correctly being loaded. But I thought that \Drupal\experience_builder\Controller\ApiConfigAutoSaveControllers::getJs() was only returning a 307 because there wasn't a modification to the JS yet.
That was wrong. The above was the real bug.
A race condition remains AFAICT
But even then … I think there's still a race condition 😬:
- loading an XB preview, and there is an auto-save/draft version of a code component
- then while I'm using XB, somebody else publishes the auto-save/draft version
- so then with exactly the right timing, I could be fetching an XB preview using the auto-save draft version of the code component, but by the time the browser requests the component's JS, it's been published, which then again results in a 307, and would still trigger the original problem!
Solution for this: don't perform a 307 redirect, but instead just serve the stored JS. That way, we can guarantee there's no scope mismatch.
→ https://git.drupalcode.org/project/experience_builder/-/merge_requests/703
⚠️ But … is the browser behavior here actually correct? 🤔 Maybe we're hitting a pretty esoteric edge case that browsers aren't handling correctly yet? (Because import maps are fairly new.)
User interface changes
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | Screenshot 2025-02-25 at 4.41.29 PM.png | 81.81 KB | wim leers |
| #4 | Screenshot 2025-02-25 at 4.31.41 PM.png | 77.63 KB | wim leers |
Issue fork experience_builder-3508922
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
Comment #4
wim leersComment #6
wim leersAnd MR for the race condition too now.
Comment #8
wim leersCrediting @lauriii for the original bug report.
Comment #9
wim leersI intend to merge https://git.drupalcode.org/project/experience_builder/-/merge_requests/702 in ~15 mins, when CI confirms it's green. It includes comprehensive test coverage. Also: @balintbrews was right all along — this is something we missed in manual testing 🙈 — and worse, the
JsComponent::renderComponent()test gave us a false sense of it actually being fairly thoroughly tested.It is tested fairly thoroughly, but it failed to account
renderComponent(isPreview: TRUE)not necessarily meaning that an auto-save actually exists.@larowlan: 👆 hope that makes sense to you too.
Comment #10
wim leersComment #12
wim leersBack to for the less critical race condition.
Comment #13
wim leersComment #14
tedbowComment #15
wim leersSo this is still open >2 months later since #12 😬
A lot of changes have happened recently on this front:
So checking this again.
Comment #16
wim leers… AFAICT this MR is still accurate? 😅
Already has the +1 from @larowlan, let's get final sign-off from @tedbow, while his head is still kinda in this space 🙏
Comment #17
tedbowTo be practical, I am going to implement my last MR comment then mark it back it as RTBC, since it is only naming comment changes.
Comment #18
tedbowMarking back as RTBC since I just changed to variable names for clarity. When tests are green I will merge
Comment #20
tedbowTested locally and the 1 e2e test that failed passed
Comment #21
wim leersThanks for renaming those variables in the test coverage — I agree it was confusing, much better now! 👍