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 😬:

  1. loading an XB preview, and there is an auto-save/draft version of a code component
  2. then while I'm using XB, somebody else publishes the auto-save/draft version
  3. 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.

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

wim leers created an issue. See original summary.

wim leers’s picture

wim leers’s picture

Assigned: wim leers » Unassigned

And MR for the race condition too now.

wim leers credited lauriii.

wim leers’s picture

Issue summary: View changes

Crediting @lauriii for the original bug report.

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

I 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.

wim leers’s picture

Title: Regression after #3500386: import map scope mismatch when auto-saved code component's JS sends a 307 » 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

  • wim leers committed 0ec7f8ad on 0.x
    Issue #3508922 by wim leers, balintbrews, lauriii: Regression after #...
wim leers’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs review

Back to Needs review for the less critical race condition.

wim leers’s picture

Issue summary: View changes
tedbow’s picture

Issue summary: View changes
wim leers’s picture

Assigned: Unassigned » wim leers

So this is still open >2 months later since #12 😬

A lot of changes have happened recently on this front:

So checking this again.

wim leers’s picture

Assigned: wim leers » tedbow
Status: Needs review » Reviewed & tested by the community
Issue tags: +stable blocker, +race condition

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.

… 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 🙏

tedbow’s picture

Status: Reviewed & tested by the community » Needs work

To 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.

tedbow’s picture

Status: Needs work » Reviewed & tested by the community

Marking back as RTBC since I just changed to variable names for clarity. When tests are green I will merge

  • tedbow committed d8f95c00 on 0.x authored by wim leers
    Issue #3508922 by wim leers, tedbow, balintbrews, lauriii: Regression...
tedbow’s picture

Assigned: tedbow » Unassigned
Status: Reviewed & tested by the community » Fixed

Tested locally and the 1 e2e test that failed passed

wim leers’s picture

Thanks for renaming those variables in the test coverage — I agree it was confusing, much better now! 👍

Status: Fixed » Closed (fixed)

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