- Issue created by @effulgentsia
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
This should update the table at the top of π± [META] Support component types other than SDC Needs work .
- πΊπΈUnited States effulgentsia
Up until now, the XB team has been following a pseudo-scrum/pseudo-kanban process, but we're now shifting into more conventional scrum. We started a new 2-week sprint last Thursday (Jan 16). I'm tagging our current sprint's issues for visibility.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@balintbrews: Did you intend for this issue to deliver end-to-end working code components, in the sense that:
- my code components appear in the left-hand in the XB UI
- I can drag them onto the canvas and see a preview
- I can populate their inputs using a field widget, and the preview updates
- I can save it
?
- π³π±Netherlands balintbrews Amsterdam, NL
#7 β¨ Create a ComponentSource plugin for JS components Active : Yes to all of those, with the following notes.
-
From the issue summary of
β¨
Config entity for storing code components
Active
:
The
status
property could potentially be used in β¨ Create a ComponentSource plugin for JS components Active to decide which code component to pick up and present as a component that can be used for creating content. Learn more about the distinction in β¨ Adding code components to components Active .I don't yet see that property added in the config entity. It's a different issue which is still in progress, but I wanted to highlight it, because it will be relevant here.
-
Before we land
β¨
Hydration library for code components
Active
, workarounds will be needed in order to see a preview of the components. From the issue summary of this issue:
Follow the steps in this comment to attach the appropriate JS files that you need to manually place in your files folder: #3483267-17: [exploratory] PoC of Preact+Tailwind components editable via CodeMirror β . We'll create a proper library to use in β¨ Hydration library for code components Active .
If this issue lands before the hydration library, we'll adjust the component source plugin as part of the hydration library issue to remove the need for the workarounds.
-
From the issue summary of
β¨
Config entity for storing code components
Active
:
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Okay! That then shows that the
// @todo Make actual render array. preview: [ '#markup' => '@todo Make something π', ],
bit in β¨ Config entity for storing code components Active should indeed point here π
-
I don't yet see that property added in the config entity.
It is β it's inherited from
\Drupal\Core\Config\Entity\ConfigEntityBase::$status
(and prescribed bytype: config_entity
in config schema). - π
-
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Updated issue summary to include items from a call with @effulgentsia
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
β¨ Config entity for storing code components Active is in! π₯³
Note: as described in the issue summary of #3499927 only if
JavaScriptComponent::status() === TRUE
should it get aComponent
config entity generated. - π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Spent some time reviewing the original POC and how it wires up Astro
I think there's probably scope to split out some smaller stories from this one and make this a parent story as follows:* Astro render element, this is reasonably straight forward in so far as it needs to take some #keys and render the twig like seen in the demo
* A library alter hook that should dynamically build library definitions based on defined code component entities - we will probably need to think about dependencies between components when one component makes use of another, but we can equally punt that to a followup
* Something similar to core's AssetControllerBase that serves CSS/JS from the assets:// stream wrapper to serve the compiled css and js as stored in the config entity. As mentioned we will likely need support for a unique hash of the contents and a preview flag. The preview flag will read from auto-saved entries. The hash will ensure we don't serve stale items. When we auto-generate libraries using the hook alter (item above) we will resolve these to the URLs for this controller. If the file exists on disk it will be served by apache. If it doesn't we'll write it to disk and then return it. This should be able to make use of a lot of the lazy asset work that landed in core in 10.1. The urls could be something like$directory_path = $this->streamWrapperManager->getViaScheme('assets')->getDirectoryPath(); $routes['/' . $directory_path . '/xb-css/{js_component}/{hash}'] //...
like we have in core in \Drupal\system\Routing\AssetRoutes::routes but rather than passing a filename in the route we could pass the machine name and hash. We can set the hash as the version in the library alter and use it in the library definition URLS.
We can then append ?preview=1 to the URL for the preview version and likely add some access controls. At some point in the future this could use the workspace ID in the URL too, to allow discrete versions per workspace. The preview version most likely wouldn't dump to file as we would expect this to be regularly updated.
For now I'll start on these in this story but it might be worth splitting them off into smaller issues if the MR size grows too large
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
we will probably need to think about dependencies between components when one component makes use of another, but we can equally punt that to a followup
+1, because that'd also require the config entity to know about those dependencies. That was not in scope for β¨ Config entity for storing code components Active (nor mentioned as future work, actually), and this is in the critical path for many other things. So let's stick to just individual code components in this issue π
Something similar to core's AssetControllerBase that serves CSS/JS from the assets:// stream wrapper to serve the compiled css and js as stored in the config entity. [β¦]
I think it could be much simpler:
- Generate those CSS and JS assets on disk, regardless of whether config is saved through user interaction or through config deployment β similar to https://git.drupalcode.org/project/component_library/-/commit/a78f3af960...
β β¨ Storage for global CSS Active
[β¦] We can then append ?preview=1 to the URL for the preview version [β¦]
Why do we even need to load these assets while previewing? That'd require going from client to server and back to the client? The client-side already has literally all the information it needs (source+compiled CSS, source+compiled JS), I don't see why the server needs to be involved at all? π€
Un-assigning @larowlan because it's a holiday in Australia on Monday. He'll likely be the one to push this forward further, but that shouldn't prevent somebody from doing so until he returns :)
- π³π±Netherlands balintbrews Amsterdam, NL
[β¦] We can then append ?preview=1 to the URL for the preview version [β¦]
Why do we even need to load these assets while previewing? That'd require going from client to server and back to the client? The client-side already has literally all the information it needs (source+compiled CSS, source+compiled JS), I don't see why the server needs to be involved at all? π€
I agree that we can keep the preview client-side. It's also how we implemented it in β¨ [exploratory] PoC of Preact+Tailwind components editable via CodeMirror or Monaco Active .
In β¨ Preview for code components Active we need to do compilation with
@swc/wasm-web
andtailwindcss-in-browser
in the browser anyway, at which point we have everything to display a preview. Like Wim said. π - First commit to issue fork.
- π¬π§United Kingdom longwave UK
Added the library hook and attached it to the island render array.
Wondering whether, similar to core, the paths should be something like
assets://xb/css/HASH.css?component=BLAH
- this way if two components happen to share identical CSS they will share the same library definition as well. Core uses the query string to build and then the stored file doesn't otherwise care.Unless we don't need the preview at all as per #15/16.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
My thinking around the client side/server preview comes down to import maps. If we need an import maps it needs to be in an IFrame. But if it's already solved, that's awesome
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Everything in this MR so far has focused on the astro island/rendering/assets aspects.
But there's more to this issue:
- The issue summary states clearly that there must be a new
ComponentSource
plugin. - I asked @balintbrews this in #7 and he responded affirmatively:
my code components appear in the left-hand Library in the XB UI
Working on those two aspects.
- The issue summary states clearly that there must be a new
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Delivered on much of #21, but not everthing yet. EOD, so passing context + next steps on to the next person:
- Managed to lift 600 LoC out of
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\SingleDirectoryComponent
intoGeneratedFieldExplicitInputUxComponentSourceBase
. I kept it as simple as possible, and resisted doing significant refactoring π€ - This made
SingleDirectoryComponent
merely ~275 LoC, and there's likely more still to be done. - Similarly, extracted the
prop_field_definitions
setting for theSingleDirectoryComponent
ComponentSource
plugin out of its config schema type into a new config schema base type: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... - Which then allowed me to quite simply/elegantly introduce the
JsComponent
ComponentSource
: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5...
Next steps:
- auto-creating
Component
config entities based onJavaScriptComponent
config entities. @longwave suggested automatically doing that whenever aJavaScriptComponent
config entity is saved. I agree that makes sense. This would be a::postSave()
override in\Drupal\experience_builder\Entity\JavaScriptComponent
. - The easiest way to test is to expand
\Drupal\Tests\experience_builder\Kernel\Config\ComponentTest
β which was previously expanded in π Add support for Blocks as Components Active to testBlock
-sourcedComponent
config entities. - π Introduce unit test coverage for both ComponentSource plugins (Block + SDC) Active would make writing tests for this a hell of a lot simpler. β οΈ But I think we explicitly should keep that out of scope to ensure we land this in the next few days rather than growing it. Let's add unit tests in #3475584.
- Docs: updating
docs/components.md
! - I'm sure there's more, but those are the first steps :)
P.S.: I did not test
\Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::renderComponent()
at all, I just matched what @larowlan did in\Drupal\Tests\experience_builder\Element\IslandTest
. It likely needs updating π - Managed to lift 600 LoC out of
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Also merged in upstream and resolved conflicts, because this conflicted heavily with π Remove references to 'props' outside of SDC - use 'inputs' instead Active .
Should make for a smooth start for the next person π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
wim leers β changed the visibility of the branch 0.x to hidden.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
SO great to see @larowlan continued where I left off π€©
- Favorite changes are the
ComponentSourceInterface
API tightenings by @larowlan β best example: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... β done in a way that does not balloon scope and https://www.drupal.org/project/experience_builder/issues/3498889#:~:text... β remains true π - Worried: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... and https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... implement the majority of
π
Capture all reasons why particular SDC is incompatible
Active
β I'd definitely have considered that out-of-scope π
That's how we went from
14 files changed, 855 insertions(+), 742 deletions(-)
to30 files changed, 1632 insertions(+), 915 deletions(-)
π³ Postponed the other issue: #3473275-13: [PP-1] Support disabled Block Components + multiple reasons for incompatibility β .
(And related: https://git.drupalcode.org/project/experience_builder/-/merge_requests/5....)
But it looks like @larowlan tackled things I had not even identified in #22, so I can literally work on the list I created there π
- Favorite changes are the
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
It looks like I was wrong β :
- @larowlan did implement the "auto-create upon save" functionality, just not in
::postSave()
, but in the newJavascriptComponentStorage
π Only thing that needed changing: respect thestatus
flag that β¨ Config entity for storing code components Active assigned a particular meaning to. - He also added explicit test coverage for it, in
JavascriptComponentStorageTest
. Still, updatingtestComponentCreation()
the way that I did provides value: it ensures that identicalprops
for an SDC and a "code component" result in the exact sameprop_field_definitions
π
I did find some problems though, and I've fixed those.
I think I wonβt be able to push this forward the next few hours. The only remaining thing I have partially (locally) is schema refinements, but thereβs plenty more to do that wonβt touch that.
- @larowlan did implement the "auto-create upon save" functionality, just not in
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Finished everything in #22, except for the docs (only a stub was added).
Iterated on everything @larowlan did overnight, and between his work and mine, we surfaced a bunch of oversights in both β¨ Config entity for storing code components Active and the way that even the foundations of
Component
config entity type's support forComponentSource
-specific settings were not quite right yet. Which is understandable, because it's this issue that's introducing a 3rdComponentSource
plugin (and as the adage goes: an abstraction requires 3 concrete uses), and one that is quite different in some ways and almost the same in others!I've spent most of today increasing test coverage, and in turn revealing weaknesses in our validation and hence the reliability of the
Component
config entity type in combination with the newComponentSource
this MR is introducing. Those weaknesses are important to eradicate to remain on track for requirement14. Configuration management
.The one bit where I'm going quite strongly against what @larowlan did last night is https://git.drupalcode.org/project/experience_builder/-/merge_requests/5..., because by using
ContribStrictConfigSchemaTestTrait
, it became clear that all of those edge cases @larowlan was testing β¦ actually are made impossible by the validation for theJavaScriptComponent
config entity, and for good reason! (See the commit message.)I believe that now, especially after the source-specific test coverage (which revealed missing validation) and its fixes (A β note how this causes the exact same fails + B) everything is in place π€
The only thing I have not reviewed at all is all rendering aspects: the astro island and asset generation. But for those, I'm happy to defer to @longwave & @larowlan. I want to get the config bits to be as robust as possible as soon as possible because they affect the data model/update path/overall stability β tweaks to the rendering can easily happen in subsequent MRs.
So: approved this MR! Hopefully @larowlan & @longwave think this is RTBC by tomorrow π€π
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
FWIW
@larowlan did implement the "auto-create upon save" functionality, just not in ::postSave(), but in the new JavascriptComponentStorage
I did it in the storage handler because we have DI in storage handlers, in Entity::postSave we have to use \Drupal. We use this pattern fairly regularly for client projects and I think it is cleaner.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
#28:: ah, of course! I somehow always forget thatβs an option ππ
Green and lots of commits overnight π€©
Reviewing!
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@larowlan opened π Allow component source plugins that don't have a plugin_id setting Active and π [PP-1] Remove more component manager overrides Active .
I opened π [PP-1] Refactor GeneratedFieldExplicitInputUxComponentSourceBase and FieldForComponentSuggester to need only SDC's ComponentMetadata, not SDC plugin instances Postponed .
The goal for this issue, confirmed by @balintbrews in #8, is this:
@balintbrews: Did you intend for this issue to deliver end-to-end working code components, in the sense that:
- my code components appear in the left-hand in the XB UI
- I can drag them onto the canvas and see a preview
- I can populate their inputs using a field widget, and the preview updates
- I can save it
?
Ideally, we'd have an end-to-end test. But I think given that 99% is identical to SDCs, we could get away with not having that, and a future issue could do that β the scope of this one is already enormous.
I think that this MR could land if:
- manual testing shows it works
- the path is paved for a future end-to-end test, by ensuring that a JS component is already present and available for placing in e2e tests
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Thanks to @longwave for getting the missing bits for Astro assets in place π He also tells me he has essentially only nits. π
Good news all around, because:
- Manual testing shows it works:
(for this demo, I only commented out theif ($this->configInstaller->isSyncing())
early return inJavascriptComponentStorage
β to force a correspondingComponent
config entity to be created π) - Pure back-end test
- End-to-end test that verifies a code component can appear in the primary panel's "Library" tab and renders a preview on hover: (failed upon adding π)
- End-to-end test that verifies a code component can be added to the canvas (failed upon adding π)
- Made the end-to-end tests pass again by actually creating the expected JS component: https://git.drupalcode.org/project/experience_builder/-/pipelines/409263
Given how many front-end issues this blocks (virtually everything in π± [Meta] Plan for code components Active ), plus it following the exact same pattern as the pre-existing test logic, plus the vastness of this MR (~2500 LoC added, net π³), I'm not awaiting FE approval for the end-to-end tests. It's too important to land this sooner rather than later!
So: RTBC'ing. Only thing outstanding: docs. First: belated lunch.
- Manual testing shows it works:
- π³π±Netherlands balintbrews Amsterdam, NL
Crediting @effulgentsia, who had the vision for this plugin and using Astro islands.
-
wim leers β
committed c08e6e3c on 0.x authored by
larowlan β
Issue #3498889 by wim leers, larowlan, longwave, balintbrews,...
-
wim leers β
committed c08e6e3c on 0.x authored by
larowlan β
Automatically closed - issue fixed for 2 weeks with no activity.