🇦🇺Australia @larowlan

Rough 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

Committed to 11.x - thanks
Discussed backport with @catch and decided not needed because this isn't a stable blocker

mcdruid → credited larowlan → .

mcdruid → credited larowlan → .

mcdruid → credited larowlan → .

Good point. Experience builder is only using it in an IFrame so sidesteps that issue

@effulgentsia pointed out that support for multiple importmao scripts is coming to browsers which would also help, but that's a way off yet

So probably worth ignoring the XB approach for now

Ignore the default value in the component above {name} = 'Steve' if I fix that to {name = 'Steve' } I get a name to show, but it's always Steve, i.e. the props still don't get passed in.

In my local testing this is allowing hydration although props aren't being passed through properly.
Ran out of time to work out what is wrong with props:

Here's my component

uuid: 221b3268-089f-48dd-ad84-52a9d9dd868c
langcode: en
status: true
dependencies:
  config:
    - experience_builder.js_component.chips
label: Chips
id: js.chips
source: js
category: '@todo'
settings:
  plugin_id: chips
  prop_field_definitions:
    name:
      field_type: string
      field_storage_settings: {  }
      field_instance_settings: {  }
      field_widget: string_textfield
      default_value:
        value: steve
      expression: ℹ︎string␟value

And my Javascript component:

uuid: 690ce531-b957-42f2-925d-cac7bd042cf0
langcode: en
status: true
dependencies: {  }
machineName: chips
name: Chips
required: {  }
props:
  name:
    title: Name
    type: string
    examples:
      - steve
slots: {  }
js:
  original: "const Title = ({name} = 'Steve') => {\n  return <h1 className=\"biggin\">Heya {name}</h1>\n}\nexport default Title;"
  compiled: "import { jsxs as _jsxs } from \"react/jsx-runtime\";\nconst Title = ({ name } = 'Steve')=>{\n    return /*#__PURE__*/ _jsxs(\"h1\", {\n        className: \"biggin\",\n        children: [\n            \"Heya \",\n            name\n        ]\n    });\n};\nexport default Title;\n"
css:
  original: '.biggin { font-size: 5rem;}'
  compiled: "/*removed a load of tailwind stuff here that I didn't add*/.biggin{font-size:5rem}"

Here's how it is rendered in the island

<astro-island uid="14806625-5dd9-4035-9da4-93cccf9859a8" component-url="/sites/default/files/astro-island/i1mwDMruPt1dJK1tlNyp7MGMN38P9JKooOtZVqAxnQc.js" component-export="default" renderer-url="/modules/experience_builder/ui/lib/astro-hydration/dist/client.js" props="{&quot;name&quot;:&quot;bobby&quot;}" client="only" opts="{&quot;name&quot;:&quot;Chips&quot;,&quot;value&quot;:&quot;preact&quot;}" data-xb-uuid="14806625-5dd9-4035-9da4-93cccf9859a8"><h1 class="biggin">Heya </h1></astro-island>

You can see the name prop is being set. but not being rendered properly

Screenshot showing 3 placed instances

With experience builder I went with a much simpler approach which is to support #attached of type 'import_maps' and then have an attachments processor that consolidated all them and made a single <script type="importmap">

Might be worth considering here too

greggles → credited larowlan → .

Git bisect shows 📌 OpenAPI spec insufficiently precise for `LayoutComponent` Active caused this

larowlan → created an issue.

Made sure we have enable/disable without the status key _and_ removed the delete operation
Added tests for both

Working on #8

Got a fix up for this

Either way we need a custom list builder to support disabling via the UI but preventing from showing up in the UI

larowlan → made their first commit to this issue’s fork.

This was introduced by https://git.drupalcode.org/project/experience_builder/-/commit/8c961df3c...

Which added support for the listing showing disabled config entities - needed for code components.

The status entity key has been in components since https://git.drupalcode.org/project/experience_builder/-/commit/5c1c06641...

We had coverage for JS, but not for the converse on components and patterns, so I've added that.

larowlan → made their first commit to this issue’s fork.

I cannot reproduce this either

I think my 'fresh install' included enabling navigation, layout builder and layout discovery but then uninstalling them.

But skipping that step, can't reproduce.

Putting this down to being under the weather.

Thought 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

Also happens on 11.1.2

My page is also mangled (although that could be an 11.x issue)

After updating to 0.x and reinstalling seeing 🐛 Warning from block components when enabling for global template Active

larowlan → created an issue. See original summary → .

Tests are passing now

I added a test to address @tedbow's comment - but it appears it hasn't gone far enough

I think #20 is ok, somewhere I saw @lauriii say having it consistent means we can record generic training videos, so that makes sense.

I think the original goal was to allow modules to have a say in all of this. But I think so long as they can put things in the extensions drawer, that need is met.

So plus one for moving ahead with #20/ #21

Implemented option 2

balintbrews → credited larowlan → .

https://www.drupal.org/project/workbench_email/releases/2.3.4 →

Cutting a new tag, thanks for taking this on

Sounds like option 2 is the preferred approach, I'll start on that

This looks good to me, I manually tested it and confirmed it is working as expected.
I also tried to remove ::findTargetForProps and confirmed we cannot do that without 📌 Split model values into resolved and raw Active

balintbrews → credited larowlan → .

https://www.drupal.org/project/workbench_moderation/releases/8.x-1.9 →

HEAD is failing, merging

larowlan → made their first commit to this issue’s fork.

Some phpunit fails, looks like I can reasonably cherry-pick the fix from 3.x, giving that a crack

Not worried about phpcs etc

Thanks mate, copied in the gitlab ci file from 3.x, let's see how that goes

larowlan → made their first commit to this issue’s fork.

Looks good to me, nice cleanup

Some follow ups from this issue
- 🐛 Deleting a content entity should clear its values from the auto-save store Active
- 🐛 Define what should happen if you delete a page while you are editing it Active

larowlan → created an issue.

larowlan → created an issue.

@akhil babu is correct, it is wrong in the config entity

drush cget experience_builder.component.block.views_block.who_s_online-who_s_online_block
uuid: 57867d30-4a30-4b86-af25-12cb6fa03497
langcode: en
status: true
dependencies: {  }
label: 'Who's online block' 👈️👈️👈️
id: block.views_block.who_s_online-who_s_online_block
source: block
category: 'Lists (Views)'
settings:
  plugin_id: 'views_block:who_s_online-who_s_online_block'
  default_settings:
    id: 'views_block:who_s_online-who_s_online_block'
    label: "Who's online"
    label_display: ''
    provider: views
    views_label: ''
    items_per_page: none

larowlan → created an issue.

Added some follow ups
📌 Introduce value objects for auto save entries, split up ApiAutoSaveController::post Active
📌 Consider adding AutoSaveData::asResponse Active

larowlan → created an issue.

larowlan → created an issue.

Rebased, should be good to go again if green

This was good to merge a week back, +1 if it still applies

At present the permission is 'access content'
You would need to implement a route subscriber and alter the route to use a different permission

Generally I would encourage that to be handled in the theme via preprocess_breadcrumb

Thanks, the fix should be as simple as adding #tree => TRUE on your element, but I would also accept an MR to make that the default

There is a core idea that has some overlap here 📌 Make Drupal core multi-frontend Active
I think we can close this but revive it in core by adding support for splitting the renderer up so that HTML is just one serialization format we can generate from render arrays.

wim leers → credited larowlan → .

smustgrave → credited larowlan → .

wim leers → credited larowlan → .

https://www.drupal.org/project/nsw_feedback/releases/2.0.0-beta4 → done

Now that 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active is in I think we should explore using these for entity form fields as well.

I could also see a world where we had server-side transforms (these would be callbacks) so that we could sidestep issues like \Drupal\Core\Entity\Element\EntityAutocomplete::validateEntityAutocomplete and the like - this would hopefully mean we could decouple from the form system and not need to build and submit a form anymore.

Fixed in dfaf6b18

I confirmed #34 solved the issue so I reverted f3fd9d2f

#34 changed the behaviour in HEAD - previously if you sent a field you didn't have access it would generate an error.

Now the field is ignored.

I updated the test to reflect this and it now passes - that is 9592dd2a

Would be good to get a +1 from Ted/Wim/Lauri/Alex on the new behaviour

This should be green again and the 2 bugs are now fixed.

Thanks @bnjmnm, we should revert https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... then as I did that as a temporary workaround

larowlan → created an issue.

This doesn't happen in head because we don't use the converter service, we use a custom convert method in preview controller

I vote we revert back to that and sort it separately

That's the style tag added in DummyPropsEditForm, it can go, it was something I tried to get the test passing

My gut feel is although it would be more elegant, it would be tight coupling.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

There were 2 methods in ApiLayoutController that had '@todo make this a trait' pointing here.
I ended up making them a trait to share with ApiPreviewController, but I like your proposal on the issue to make it one controller.

Needed more of this in 📌 Support server side massage and validation of component prop form values Active

Can I suggest something other than propSources - we'll likely want to ship jsonSchema for block components - and currently this sits under field_data.

Or if we rename it to propSources, can we move the schema elsewhere?

Also blocked on 📌 OpenAPI spec insufficiently precise for `LayoutComponent` Active which will make the diff here smaller

Addressed #18 - it is much simpler than that now that 📌 Maintain a per-component set of prop expressions/sources Active is in, we just send the model. That's it. Rebased, then squashed to do that and then removed 📌 Include the preview HTML in the layout controller payload Active

Will start on the review feedback and the remaining tasks (including fixing tests).

Commit ace768f6 has the work here on top of 📌 Support server side massage and validation of component prop form values Active

When that is in, we can rebase --onto origin/0.x and drop d78ade44

Opened the source code of @swc/wasm-web and it has this

async function __wbg_init(module_or_path) {
    if (wasm !== undefined) return wasm;


    if (typeof module_or_path !== 'undefined') {
        if (Object.getPrototypeOf(module_or_path) === Object.prototype) {
            ({module_or_path} = module_or_path)
        } else {
            console.warn('using deprecated parameters for the initialization function; pass a single object instead')
        }
    }

    if (typeof module_or_path === 'undefined') {
        module_or_path = new URL('wasm_bg.wasm', import.meta.url);
    }
    const imports = __wbg_get_imports();

    if (typeof module_or_path === 'string' || (typeof Request === 'function' && module_or_path instanceof Request) || (typeof URL === 'function' && module_or_path instanceof URL)) {
        module_or_path = fetch(module_or_path);
    }

    __wbg_init_memory(imports);

    const { instance, module } = await __wbg_load(await module_or_path, imports);

    return __wbg_finalize_init(instance, module);
}

export { initSync };
export default __wbg_init;

so the default export takes a module_or_path

We're calling it with no arguments

But we should be able to pass in the path there and it will end up with

module_or_path = fetch(module_or_path);

I think we can use Rollup's replace plugin here (unless I don't understand the problem).

We can use __DRUPAL_XB_BASE_PATH__ in all of our code and in our viteconfig we can pass config to rollup plugin replace to replace that with a reference to a global, e.g. drupalSettings.xbBasePath that we ensure is always available on the page via #attached

Agree re 📌 [PP-2] Remove InputBehaviorsBlockSettingsForm and consolidate with input components form Active - added it

Yes I think this can land and I think some of the changes in 📌 Will passing the model props for a component via GET parameters hit limitations due to URL length Active highlight that we need to make sure the #parents are consistent - in fact they should be applied in the ComponenteInputForm rather than in each source plugin. That will move this closer I think - will try to take a stab at this before the end of the week

hooroomoo → credited larowlan → .

wim leers → credited larowlan → .

This is for when we fetch the component inputs form

x-post

I have an MR up on 📌 Will passing the model props for a component via GET parameters hit limitations due to URL length Active 🤞

Ok, added a hidden field with those values to ensure it works with AJAX.

Yes these can be tampered, but so could the URL so 🤷

Maybe we need an anti-tamper hash?

Yep, doesn't work with AJAX forms because the form needs to be rebuilt and that requires the params - and they are no longer in the body of the request because now we're POSTing the Drupal form

That was far too easy