🇦🇺Australia @larowlan

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.

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

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

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

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']);

ADR/discussion/research on SSR is here for review

poker10 → credited larowlan → .

Added #3530795-6: [needs design] Invalid prop errors should be detailed in Component preview; otherwise the preview LIES 😱 →
Added 📌 Narrow the conditions under which we allow a prop to be an empty string Active and moved the MR over there
Added 📌 Lift HTML5 validation errors into the formState slice Active and moved the MR from 🐛 UI change in 3529788 allows publishing components with empty required fields Active there

larowlan → created an issue.

larowlan → created an issue.

Per #3529788-32: Required `type: string` (without other constraints, so "prose strings") props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases → we also need to take into account when zend.assertions are set to 1 as that will also cause a render fail from \Drupal\Core\Template\ComponentsTwigExtension::validateProps which is dynamically injected into each component's twig template via \Drupal\Core\Template\ComponentNodeVisitor

Even if XB doesn't validate component input when creating the preview, that code will when assert is enabled.

We could either:

• Detect asserts are on and swap out the validator to do nothing on preview routes, OR
• Adapt the 'component failed to render' message to include assert is enabled messaging

I would vote for the first option as having to disable assert during development could be silently muting other errors

Screenshot for #32 showing preview still displays with invalid data in link field

Re #30 - I've confirmed that the error only occurs if you have zend.assertions set to 1

With it set to -1 (even with the deprecated ini_set('assert.active', 1); in experience_builder.module) there is no error.

That's because \Drupal\Core\Template\ComponentsTwigExtension::validateProps only runs inside an assert function.

So that might also be something we should consider in 📌 Invalid prop errors should be detailed in Component preview Active

With that in mind and because 🐛 UI change in 3529788 allows publishing components with empty required fields Active is in - I am going to mark this back as fixed and open follow-up issues for the two follow-up MRs here as well as comment w.r.t zend.assertions on 📌 Invalid prop errors should be detailed in Component preview Active

wim leers → credited larowlan → .

Sweet

V5 doesn't have that issue, if you're not using any custom code with the php API of some of the v3 services you could consider moving to v5

Ah in #20 I had looked at the wrong MR and missed @wim leers's

That approach is fine with me, it is leaning on existing definitions of `isEmpty` in the field items inside the prop source, which is kind of what I was trying to achieve with the initial approaches of calling validate on that field item list in #20

I'll close my MR, sorry for the crossed wires.

I'm not sure at that point what we can do to recover, so that means we likely need to start doing input validation before preview generation

It may be that these errors are only coming from \Drupal\Core\Template\ComponentsTwigExtension::validateProps in which case that shouldn't happen in production when `assert` should be disabled.

We might need to swap in our own validator on preview routes to prevent it during development and tests where assert is likely enabled

larowlan → created an issue.

Left a couple of comments on the MR

Have asked other committers about backport here

#12 sounds like we don't have any issues with other modules.

The fact we're getting TRUE from the EH builder indicates it should be building the hierarchy.

Is it possible your tree is corrupt? There's a drush command to rebuild it, but it will be empty whilst that is occurring, so it might be best to do that outside times where the site is under load.

Committed to 11.x - thanks!

Added a fourth (!) approach in MR 3537154-how-about-non-blank

I think that if invalid data is not flagged by calling validate then we have reduced trust in our systems.

The first approach I took was to add the NonBlank constraint to StringItemOverride on the value property. But then I realised we never call validate on the field item list we have inside StaticPropSource so that didn't do anything.

So then I added a validate method to PropSourceBase and had \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::validateComponentInput call that. But this blew up spectacularly because the field item list we have there has no entity parent and \Drupal\options\Plugin\Field\FieldType\ListItemBase::getSettableOptions assumes we do.

So then I backed all that out and instead injected the basic validator into GeneratedFieldExplicitInputUxComponentSourceBase and validated against a NonBlank constraint for our required props in ::validateComponentInput - which gets to the root of the issue - that json schema doesn't consider '' as invalid for a required items - whilst we do.

That got the test from @tedbow to pass.

Sorry for adding another approach, I'm just concerned about the number of workarounds we are doing here.

In response to #23 and #24 I think as a minimum we should be displaying and error banner and retaining HTML5 validation errors on the form using the existing styling. So most of my MR but without the early return in the commitFormStateToStore method.

The thing I'm confused by is that we don't PATCH to the backend if the field you're editing is invalid - inputBehaviors doesn't call the commitFormStateToStore method if validation fails.

So I'm not sure why we would want to do that if you edited a different field.

i.e. I don't see the point of having validation if you can bypass it.

I think it's simple to reason about what we do to show a preview in the case of an empty required string like the original MR did (FYI this wasn't an issue until the changes in ✨ Add a Video prop type to the Code Component editor Active that I've reverted in my MR).

But what about this scenario?

1. Add the hero
2. Edit the href to 'not a link'
3. see the validation error 'data must match format "uri"
4. edit the heading which bypasses the validation error
5. Receive the component failed to render error
   

In that scenario what would we do to render the preview? We don't do any validation on the preview generation so we don't know things are invalid until we hit the catch block in the RenderSafeContainer element. I'm not sure at that point what we can do to recover, so that means we likely need to start doing input validation before preview generation and in case of failure fall back to the example value. Up until this point we've not wanted to do validation on preview for performance etc.

greggles → credited larowlan → .

wim leers → credited larowlan → .

Seeing this again in CI today https://git.drupalcode.org/project/experience_builder/-/jobs/5979572#L1351

I've opened I added an MR for a different approach against 🐛 UI change in 3529788 allows publishing components with empty required fields Active in which is basically #21

Here's a screenshot showing the form preventing submission until any errors are resolved.

We already had code for setting formErrors in the formState slice, but weren't using it for HTML5 validation, the MR adds the e.target.validationMessage to the errors in formState slice so they persist and we can query for errors in other fields in the commitFormState callback and return early.

This also lets us show a nice user facing error to 'resolve the errors' as we can also query the slice

I added a different approach in https://git.drupalcode.org/project/experience_builder/-/merge_requests/1342 which is basically #12 and #3529788-21: Required `type: string` props should fall back to empty string (even though invalid) while Content Author is typing, to allow matching live preview in most cases →

I think the root cause of this was these two commits added late to ✨ Add a Video prop type to the Code Component editor Active

1. The changes to StaticPropSource here
2. The test changes here to make the above change pass

I think we should consider reverting these two commits (the first one only for changes to StaticPropSource).

Additionally, I think we should revisit the approach we took in 🐛 Hero component failed to render after removing text Active - I've reopened that and commented over there with my reasons - tl;dr I don't think we should allow the user to bypass clientside validation by editing any other field.

Just catching up on this via 🐛 UI change in 3529788 allows publishing components with empty required fields Active

Late to the party but

Per @lauriii
It should be possible to enter an empty string in a required text field, such as the heading prop in the hero component then

when that prop is emptied the Preview should still update -- it's fine that it is showing an impossible-ish state due to an empty required prop

I think the approach we took here might not have gone far enough - in this case ajv validation is marking the field as invalid and we should not update the client-side model or send the PATCH request back to the backend/store autosave/update the preview. But that doesn't happen on its own

I _think_ the root issue here was with these steps in the OP that allow a user to bypass clientside validation:

Remove heading text, and sub heading text

In so far as this is the order of operations:

1. Remove heading text 👈️ this puts the form into an invalid state
2. and subheading text 👈️ the input behavior triggers the PATCH/ autosave etc because a prop was updated But it does not consider the validity of the form

i.e. a user can edit any other field to bypass our clientside validation

And then in terms of the root cause of 🐛 UI change in 3529788 allows publishing components with empty required fields Active I think it was these two commits added late to ✨ Add a Video prop type to the Code Component editor Active

1. The changes to StaticPropSource here
2. The test changes here to make the above change pass

i.e. I think we should consider revisiting the approach in this issue, and reverting those two commits from ✨ Add a Video prop type to the Code Component editor Active .

In terms of revisiting here, I think we should prevent PATCHing/autosave/preview from happening if the form is invalid.

Without that, we're basically rendering our use of ajv/clientside validation redundant - if a user can edit another field to dismiss validation - there's no point to it and it doesn't give us any protection at all.

I'll put the comments specific to 🐛 UI change in 3529788 allows publishing components with empty required fields Active over there too.

Sorry for reopening a can of worms.

hooroomoo → credited larowlan → .

@dcam came on as co-maintainer for the aggregator module when it moved to contrib.

He completely transformed the state of the module and worked to resolve all open bugs.

He was a pleasure to deal with

I would welcome his addition to the subsystem maintainers team - +1

Going to pause and create an ADR around the requirements here and for SSR

Rather, any element that is added via a #process callback does not get run through FormIdPreRender::addAjaxAttribute and as a result does not have the form-identifying attribute

Yes, @effulgentsia pointed me towards that when I had a similar issue in ✨ Add a Video prop type to the Code Component editor Active - the same issue was occurring with the generic file widget. A form of that went in with that issue, so this may be resolved now.

Whoops I forgot that, thanks

Committed to 11.x and backported to 11.2.x - thanks folks

Congrats on your first core issue credit @savage1974 🎉

Left a question on the MR with an idea to make it automatic for any command using the trait

Does step 7 still occur after 🐛 Page data form values-in-progress not retained for Media Library (and perhaps other) fields Active - are you able to retust please @neha_bawankar ? thanks!

@vasantha deepika I think this applies to the 'xb_page' entity type. You can add a new page by choosing 'new page' from the dropdown at the top of the editor

This is undoubtedly to do with cache tags/invalidation

Thanks for filing this @heyyo - I wonder if you have any ideas for proposed resolution.

I wonder if we can make use of a meta property on slots in the definition to flag a slot as inline?

In terms of the space, this is needed for the sake of drop-targets. I guess we could hide that unless something was being dragged but that might make dropping difficult if when drag start occurs we added back collapsed/empty slots

Is this when applying the recipes? Because since ✨ Add automated image optimization to image component Active the recipes no longer enable xb_stark - I wonder if that's related?

We fixed the attribute addition in ✨ Add a Video prop type to the Code Component editor Active - so this might be solved - can you retest and advise?

Pretty sure this was solved in ✨ Add a Video prop type to the Code Component editor Active - can you retest and confirm?

I have seen this regularly in e2e tests - example - https://git.drupalcode.org/project/experience_builder/-/jobs/5966903#L4919

It appears to be random - as doesn't occur on re-test - but that might indicate its a race/timing issue - and being related to 'changed' that feels likely as some time passing may be the trigger.

larowlan → changed the visibility of the branch 3514910-pp-1-real-time-preview to hidden.

Does this still occur after 🐛 Page data form values-in-progress not retained for Media Library (and perhaps other) fields Active - are you able to re-test @neha_bawankar ?

looks like more form cache/build ID shenanigans

I wonder if this still occurs after 🐛 Page data form values-in-progress not retained for Media Library (and perhaps other) fields Active - are you able to re-test @neha_bawankar ?

This is most likely that the form build ID in the page is tracking the auto-save version but no longer exists once the entity is published.

i.e. the form cache is lost because of how the EntityFormController works. I think this should be a stable blocker, adding tag.

I would suggest that you add a caption prop to the component (SDC or code) and render the markup as follows if you want to provide a text-only alternative to the video.

<figure>
  <video .../>
  <caption>{{ caption }}</caption>
</figure>

Failing that you can make use of the track element if you wish to add e.g. subtitles/timed text tracks

@heyyo is that against 0.x? Can you share that SDC (or point me towards somewhere it lives in a repo)?

@jessebaker this was merged is 'Active' the correct status?

This is green and ready for review w/ test coverage

Pushed a fix with some expanded docs of what is going on in that method.

As the bug is in \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItemListInstantiatorTrait::generateComponentTreeKeys, this also impacts ContentTemplates - new title

Was able to write a test using @thoward216's excellent sleuth work at #16 - confirming this is not related to 🐛 ApiLayoutController::getRegionForComponentInstance doesn't work well with nested components if there are multiple regions. Active or 📌 Ensure predictable config export order of config-defined component trees Active

Pushed a failing test replicating #16

Will try to move this along a bit while @thoward216 sleeps 🧚‍♀️

Can you give the MR a test @heyyo to see if it improves things?

Left a review 👍️

Left a comment about how I think we can fix this in a more generic way

Are you thinking we only do this when we're previewing inside of XB? Or when viewing the page on the published site outside of XB as well?

I was doing it for both. I hadn't finished wiring up <xb-preview> yet, but the SDC elements were working as-is (as you'd expect). I'd still like to explore this approach as it sidesteps the need to detect use client and unwrapping altogether.

I think the earlier MR here (before I derailed us with comment #9) was on the right track. However, I think the logic needs to be: if the component has the 'use client'; directive, or if one of its dependencies does.

Ok, sounds like this is the next step for this issue.

I just got is that ultimately what we're trying to emulate is SSR. So, what if we make emulate that better, by using renderToString() (same as server.js within @astrojs/preact) and then replacing its .outerHTML with that? This would result in the rendered HTML getting inserted into the DOM just once.

Yep, we could do that but we'd still have to make the decision about interactivity with use client.

So it sounds like the next logic steps here are:

1. Change the logic back to detect 'use client', but also consider dependencies AND useSWR or next-image-standalone as a trigger for preventing unwrap
2. Explore using renderToString to replace the outerHTML rather than rendering and then moving

The questions is, how much of this is moot if we have SSR? Should we be focusing on that instead? I think we can make that pluggable so that there are multiple SSR providers - e.g. symfony/process that calls node on the host, a HTTP endpoint that calls a separate container running an express web-service etc.

Now that ✨ Unwrap astro-islands and astro-slots Active is postponed, we don't need to postpone this on that.

Got a working version of this in the branch

both require a DOM element container that they can completely take over

I've blown away my local work but should be able to revive it from phpstorm local history.

my idea was to add an xb-preview custom element at the root of the ComponentTreeItemList::renderify call and use that as the container that is taken over.

I replaced <astro-island> with a <template data-xb-island> element but retained all the attributes, after renaming them to have a data-<code> prefix. I also changed the <code><template data-astro-slot> to be data-xb-slot

In the custom xb-preview element I was planning on using querySelectorAll to find all <template data-xb-island> and then looping over those to Promise.all on their imports.

I needed to copy astro-island's 'reviving' of properties wholesale, but that's not a lot of code.

Where the hard-coded importing of astro's preact crept in was the renderer's 'static-html.ts' - and of the h function although I note that they also have similar functions for other their other framework renderers.

The idea was to build a single component tree with the SDC and block components alongside the JS components and then use the preact render call on that inside the top level xb-preview custom element. Its a bit like what we're doing with hyperscriptify but on the preview side.

If you think this is worth exploring further I'd be happy to, I timeboxed it to ~90 mins because I didn't want to go too far down a rabbit-hole if the result was too rigid/limiting of future options.

I could see us having a factory of renderers based on the framework option, it does appear that most of Astro's renderers follow a similar pattern.

As per above I think we need to document all the use-cases here.

Astro uses the display contents approach, maybe that's where we leave this - i.e. we mark this as closed won't fix.

I did a short spike on #26 and whilst I think it is feasible, it would bind us to too tightly to preact and the internals of astro which would make supporting other frameworks more difficult in the future.

Thanks sun for helping me find my feet in core contribution all those years ago

I think there is a minor impact to performance as a result as when the iframe swaps you can sometimes still see the astro components load in

Should we open a follow-up to try and make the 'iframe swap' wait for the `astro:hydrate' event - alternatively we can wait until iframe.contentDocument.querySelectorAll('astro-island[ssr]') has a length of zero as that attribute is removed after hydration

I have an alternate idea here.

The reason this is forcing us to chose between unwrapped and not is because we're treating each component as its own island and the unwrapping code is doing insertBefore rather than moveBefore which means we're losing events etc. moveBefore is only supported in chromium based browsers so we can't use that.

I think we can combine this and ✨ Real-time preview for props changes of JS components Active if we expand the size of our island to include the whole preview, rather than one island per component.

With the wrapping islands we have the css impact (that we're working around with display: contents now). Without them we lost interactivity and also make things harder for ✨ Real-time preview for props changes of JS components Active .

But if instead we have one island we can retain the wrapper and the interactivity and possibly make ✨ Real-time preview for props changes of JS components Active simpler.

I'll do a spike on that approach to see if it's viable. In researching ✨ Real-time preview for props changes of JS components Active I've read the internals of astro and the preact client, there's not much code there, I am confident we can fashion something that works for us.

I feel like I missed a trick not naming the element <phantom-island>

Folks, let's stay civil here

There are multiple places we call initContextual, we've updated one of those calls to check the element exists, but not the other.
But we've also updated the logic inside initContextual - I don't think we need both.
I'm inclined to make it check it from the outside rather than inside

Let's do that - thanks!

Left a question on the MR

Left a review

Current train of thought on this

* When we're in preview, have the unwrapped-astro-island add a template element with its props and children on it during unwrap. Place another template element after the last child so we have markers for where the replaced (unwrapped) HTML goes to and from.
* In XB UI emit a custom event on the active iframe preview in the inputBehaviorsComponent input form when a component prop changes _and_ it has passed validation _and_ we know it doesn't need a server side trip (can probably assume this will be fine for any scalar prop. If the event from that
* When we're in preview have unwrapped-astro-island and xb-island listen for these events and update their markup - in the case of xb-island by something similar to #14 but hopefully without needing to know the internals of preact. In the case of the unwrapped island by doing something with the stashed template elements

Will continue on monday