- Issue created by @larowlan
- Assigned to larowlan
- 🇫🇷France pdureau Paris
I am copy pasting my slack message here, because I belive it is a better place to have such dicussion.
The goal is for within Experience Builder UI to be able to edit the data that feeds an SDC's prop values and see an immediate preview of that while editing. That experience would be smoothest if the SDC's Twig was also available as a JSX component.
Compiling Twig to JSX sounds like a complicated and risky task. The models don't fit, for example:
- In JSX, no proper slots but the single special children prop instead
- In JSX, props are only scalars (because they must fit in an HTML attribute)
Also, we will meet the issues mentioned in Slack:
- by Mateu: how to port specific Twig filters, especially the ones related to Drupal API,
attributes
or any object implementingRenderableInterface
? - by effulgentsia: Twig is treating the markup as text, React is treating it as vdom object, so how to port something like this {% if x %}
{% else %}
{% endif %}?
Why not embracing the current "back to hypermedia" trend and keep the rendering server side? We already have a proposal about HTMX 🌱 [Plan] Gradually replace Drupal's AJAX system with HTMX Active replacing the AJAX system, and we can start from this or any similar technology.
While keeping the goal of live editing with immediate previews, it will solve the dual (server/browser) implementation issue, with simpler codebase and better performance.
- Assigned to bnjmnm
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
First: thanks for opening this issue! 😄
Do you know about https://www.drupal.org/project/jsx → ?
@bnjmnm has built a working PoC for this, with Drupal-defined (Form API-defined) field widgets being rendered — unmodified! — in React. See https://git.drupalcode.org/project/experience_builder/-/commits/eb-form-....
IIRC there also was a demo of the JSX theme engine rendering Twig and React components interspersed — a tree of both React components and Twig templates, with no restrictions on which is where.
Assigning to him to respond here :) Would be a shame to duplicate work.
- 🇫🇷France pdureau Paris
Another solution for dual rendering (server side / client side) would be to compile the renderer service + template engine into WebAssembly and to run the exact same code server side (wasm loaded by PHP) and client side (wasm loaded by the browser JS engine).
I already have a working implementation to show if you are interested.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Interesting, @pdureau, I'd definitely love to see that! I defer to @jessebaker, @bnjmnm, @effulgentsia and @lauriii to decide whether that's a direction we should pursue.
- 🇫🇷France pdureau Paris
Interesting, @pdureau, I'd definitely love to see that! I defer to @jessebaker, @bnjmnm, @effulgentsia and @lauriii to decide whether that's a direction we should pursue.
Too bad we are not meeting Brussels today ;)
I did a demo yesterday to some people from #experience-builder channel. Here are my notes.
I am glad my https://dilla.io/ was well received. And I strongly believed we will leverage WebAssembly in Drupal in the future. However, I am not promoting this solution right now:
- If we run the same Wasm file (with template engine and renderer service) in browser and server, we will need a WebAssembly runtime to be installed and run as a distinct OS service, because PHP is not shipped with a native WebAssembly implementation yet.
- For the same reason, I am not comfortable with a solution where a JS runtime is needed server side. I understood we all agree with that, right?
- If we ship only the template engine in the Wasm file, and run it only in browsers to render the Twig templates (we keep the twigphp one Drupal side), i doubt processing the templates without the related rendering service will work with demanding real life scenarios (with attachments, embedding and nesting of renderables, inline templating, cache management, libraries bubbling...).
- For the same reason, I am regretting that the impressive, innovative (each one took a different approach! that's great!), demos of today (as far as I understood them) were all focused on processing the template without the rest of the rendering tasks.
So, I am still in favor of keeping (with some improvement) the rendering server-side only with a HATEOAS / HTML fragments solution "à la" HTMX or Hotwire / Turbo, is a better, simpler, safer, solution.
- If we run the same Wasm file (with template engine and renderer service) in browser and server, we will need a WebAssembly runtime to be installed and run as a distinct OS service, because PHP is not shipped with a native WebAssembly implementation yet.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@pdureau fortunately got to meet a few days later :)
- Issue was unassigned.
- Status changed to Postponed
6 months ago 2:26pm 11 June 2024 - Assigned to effulgentsia
- Status changed to Active
3 months ago 12:42pm 23 September 2024 - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
📌 Add component instance edit form to contextual panel RTBC lifted relevant parts of #4's https://www.drupal.org/project/jsx → into this module.
AFAICT that means this is now irrelevant.
While
/themes/engines/semi_coupled/README.md
exists in the XB module, I think now the time has come to expand this to:- an explicit
/docs/Redux-integrated Field Widgets.md
, for better discoverability - an accompanying
Redux-integrated Field Widgets
issue queue component - Retitling the
/CODEOWNERS
entry accordingly:
[semi-coupled theme engine] @bnjmnm @hooroomoo @effulgentsia /src/Theme/XBThemeNegotiator.php /themes/engines/semi_coupled/ /templates/form/ /ui/src/components/form/
Thoughts, @effulgentsia?
- an explicit
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Discussed with @effulgentsia. He's +1 to the overall proposal. He does think the current title is too narrow though. I'll think about that some more while I get an MR up :)
P.S.: conceptual sibling issue: 📌 Clarify the "shape matching" bits: namespaces, `CODEOWNERS` and as issue queue component Fixed .
- 🇺🇸United States effulgentsia
The title I'm thinking of is maybe something like "Redux-integrated XB-cohesive Field Widgets". Maybe there's a better term to use than "XB-cohesive" but the concept is that we want the field widgets rendered as React components not only for the Redux integration, but also to be able to style them and/or add JS behaviors to them in a way that makes the props edit form look and feel cohesive with the rest of the XB app. While it might be possible to achieve that with Drupal's traditional JS/CSS/library system, I think it's easier to achieve it via the same React toolchain (JSX, CSS modules) as the rest of the app. Maybe this is moot though if the Redux integration alone sufficiently justifies and clarifies the need to render the widgets as first-class React components.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
wim leers → changed the visibility of the branch 0.x to hidden.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
- Merge request !339#3446434: Document "Semi-Coupled Theme Engine" and "Redux-integrated Field Widgets" components → (Closed) created by wim leers
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Thanks to these 2 commits:
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...
- https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... + fix an oversight in that commit: https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...
The
experience_builder.module
file is now roughly half as large as it used to be — from 583 lines to 317 lines. If I apply the same pattern to the 3 hook implementations for theShape matching
functionality, it'll go down to 224 lines. Each of the 3 include files then is 100–200 LoC.Regardless of the size of each individual file, the more important objective is that this makes it much easier to convey what all these hooks are for:
- thanks to the filename of the
*.inc
file - thanks to the
@see
pointing to the issue queue component - thanks to the
@see
pointing to the docs
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Since #18, I've:
- started an incomplete
/docs/redux-integrated-field-widgets.md
- started an incomplete
/docs/semi-coupled-theme-engine.md
(based on/themes/engines/semi_coupled/README.md
!) - updated
/docs/data-model.md
- updated
/CODEOWNERS
I still need to get tests back to passing, but only a single test is failing.
@bnjmnm is the person best positioned to complete the first two points.
This also really needs @bnjmnm's +1 for overall direction. But at least this MR now makes it clear how I think this should evolve, to better allow onboarding/participating/understanding of this critical piece of Experience Builder!
So assigning to @bnjmnm for review 😊
- started an incomplete
- First commit to issue fork.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@bnjmnm had a hunch: maybe
experience_builder_module_implements_alter()
not living in the*.module
file made a difference in preprocess hook execution order? Moved back to the*.module
file temporarily: it did not make the test pass.I had one more hunch: just move the preprocess hook back to the
*.module
file temporarily: it did not make the test pass.I don't know what tiny detail is causing this to fail tests, but it sure as hell is massively distracting. So…
require_once
+@todo
it is: https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... — we can figure out that part later. - 🇬🇧United Kingdom longwave UK
I figured out the breakage, I don't think hook_hook_info is intended for this situation and require_once is fine.
- 🇬🇧United Kingdom longwave UK
Semi-Coupled is a strange term to me; this is bikeshedding but should this just simply be called the "Twig JSX" theme engine, given those are the two things it supports? I even wonder if one day it's worth spinning out to its own module as it might be useful outside of XB for other projects that want to use React in Drupal.
- 🇺🇸United States bnjmnm Ann Arbor, MI
I'm sure there's more work to be done on the Semi Coupled docs, but this is a good point to get other eyes on it then I can address points of confusion and other feedback.
- 🇬🇧United Kingdom longwave UK
I discussed the approach so far of the new theme engine and widget implementations with @bnjmnm. While neither of us are entirely happy with the current implementation of the theme engine, it's certainly good enough for now in order to unlock the ability to build forms in Drupal and present them inside React. The technical details can be cleaned up and/or improved later, I don't think anything that's been done so far has painted us into a corner in any way. I think it's possible that we may not even need a separate theme engine here and could decorate various theme-related services in order to extend them for XB to consume, but for now we have bigger problems to try and solve.
The widget implementations are tricky: the goal is to find a generic solution to map any Drupal widget into a set of React component props, and back again, ideally without requiring custom code on the React side - we both agree that the solution should lean into the server side where possible for mapping data back and forth. This will help contrib and custom code developers avoid having to write any React to integrate with XB.
- 🇺🇸United States brianperry
Reviewed and commented on the in progress docs MR.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@longwave in #26: thank you for summarizing your high-level perspective after digging into this together with @bnjmnm. That's very helpful, and already helps make it easier for additional contributors to get a sense of where this is at and where it's going :)
@brianperry Thanks for reviewing these docs at this early stage, you asked a number of good questions that should make these docs more easily digestible for the next person! 😄👍
@bnjmnm This is definitely on the right track! 👏 I think there's sufficient feedback from @longwave, @brianperry and I now that it's worth you continue this and push it across the finish line. Based on @longwave in #26, it sounds like the two of you discussed the Redux parts and have some clarity/perspective on how that should evolve. Now let's make sure that is captured in the docs too 🙏
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
(I've told Ben this previously in a meeting, and I failed to record this in public. 🙈 Rectifying that now!)
I also expect these docs to outline that we will add support for:
- field types with multiple field properties (for example:
\Drupal\datetime_range\Plugin\Field\FieldType\DateRangeItem)
- field widgets with multiple form fields that together define a single field property:
🐛
Premature prop validation can break the UI
Active
(for example:
\Drupal\datetime\Plugin\Field\FieldWidget\DateTimeDefaultWidget
for\Drupal\datetime\Plugin\Field\FieldType\DateTimeItem
) - multiple-cardinality fields: #3467870-16: Support `{type: array, …}` prop shapes → (for example: a list of images for an image gallery SDC, a list of strings for a "recipe ingredients" SDC, etc.)
- generalizing computed field properties that do use AJAXy widgets (currently:
MediaLibraryWidget
and its hardcodeddata-media-file
attribute added byexperience_builder_preprocess_media_library_item__widget()
and hence requiring AJAX) - computed field properties for non-AJAXy widgets (for example: the
processed
field property of\Drupal\text\Plugin\Field\FieldType\TextLongItem
)
… perhaps more in the future.
If there's a solution that @bnjmnm already has in his head, then I'd like a high-level explanation for it in this MR. When there's an issue link, I'd like to see that in the
docs/*.md
file (just like we did fordocs/data-model.md
— you can find the missing/uncertain bits by looking for the warning sign emoji: ⚠) - field types with multiple field properties (for example:
- 🇺🇸United States bnjmnm Ann Arbor, MI
There might be glaring issues I failed to notice due to documenting-fatigue but I think this is a good point to officially NR it. It's a huge step in the right direction, so perhaps the main goal here could be making sure nothing is flat-out wrong. We can iterate on the quality later (and as the functionality evolves)
- 🇳🇱Netherlands balintbrews Amsterdam, NL
I reviewed the docs, improved punctuation and language, added a couple of links, and extended the
hyperscriptify
example code snippet. I think the content has the right level of details and is easy to follow. I hope that's helpful! 🙂Reassigning for reviewing backend changes.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I've seen:
- React element
- React component
- JSX template
- JSX component
- … and more
And AFAICT they're all pointing to the same thing? That's confusing, especially for somebody not deeply familiar with React/JSX. Let's use one term consistently 🙏
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
I've addressed most of my own feedback.
Most important changes:
- added context/explanations that I felt were missing
- enforced consistent use of the same terms, which I believe reduced ambiguity and hence makes this simpler to understand
- defined a few additional terms, again to reduce ambiguity
- completely overhauled the overview of what works vs is still outstanding on the Redux integration side, where I identified what I think are the 6 axes for which we need to get all permutations working.
I think the only thing that remains is for @bnjmnm to review (and hopefully approve) the MR — it's totally possible I introduced factual errors, especially WRT #34 and in the last bullet above.
Actually, the updates to
semi-coupled-theme-engine.md
for #34 are incomplete (I didn't want to push this too much in the wrong direction 😬😅). But the updates toredux-integrated-field-widgets.md
are complete, and should only need review/approval/corrections. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
wim leers → changed the visibility of the branch 3467843-backend-route-to-allow-logging to hidden.
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Hopefully the combination of:
- the 2 commits I pushed
- the 4 threads I opened covering the areas I feel the most uncertainty/confusion about
… will allow you to leave this in a place with little additional of your time, but where you also feel better about the state of it than before 🤞
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
Not all of my feedback from yesterday has been addressed yet — reflecting in issue metadata — whoops!
- 🇺🇸United States bnjmnm Ann Arbor, MI
Good News I think this image helps make sense of some of the more confusing bits
Bad-ish News (more work.. but better code) Seeing it mapped out like this made it clear there are some additional changes I should probably make to make the DX less confusing. - 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@bnjmnm that image! 🤩 👏
Observations/questions:
- Could you add lines from the
$variables
(first column) to the-xbxb.html.twig
file? (second column) I think this will also make it clear that it's possible that not every variable is necessarily consumed. - In the 3rd column, where it says , I suggest to clarify that to
- In the 4th column, why is
data-drupal-scriptified
there? That only happens on the client side, and AFAICT this image is illustrating what happens on the server side? - … which brings me to my next point: it'd be great to add a 5th column, to show what transformations happen on the client side during the "hyperscriptify" step.
- … and a 6th column that shows the end result in the widely known an lauded React Developer Tools, i.e. the resulting structure in the tab as shown here: https://react.dev/learn/react-developer-tools
(Reminds me of https://wimleers.com/talk/drupal-8-render-pipeline by the way :D)
- Could you add lines from the
- 🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
@bnjmnm already leaped ahead in #45 and created a new MR assuming 📌 Introduce the `xb_stark` theme that uses the Semi-Coupled theme engine, removes the need for taking over the Twig theme engine for the whole site Active would land any moment. Good call! 😄👏
That now did land, so I believe that we can now prominently see the light at the end of this Alice-in-Wonderland-like tunnel 😁
- 🇺🇸United States bnjmnm Ann Arbor, MI
In 📌 Create extendibility proof of concept that also serves as documentation-by-example Active I've added a test module that provides a barebones implementation of the Semi Coupled theme engine . I think we're running into a usefulness plateau with attempting to explain it all in a single document, and routing some of that documentation to the test module might prove more helpful.
I also added 📌 Create readme for hyperscriptify library Active to create a Readme for Hyperscriptify so that explanation lives within the package that provides the functionality. We can certainly link to it etc but I think having it with the package can help demonstrate that the functionality it provides is not reliant on XB or Drupal.
- 🇺🇸United States effulgentsia
Discussed this with @longwave, and one big next step here besides general review is to create tests that surface the difference cases where there isn't a 1:1 map between field values, prop values, and widget values. @longwave offered to start on that.
- 🇺🇸United States bnjmnm Ann Arbor, MI
Discussed this with @longwave, and one big next step here besides general review is to create tests that surface the difference cases where there isn't a 1:1 map between field values, prop values, and widget values. @longwave offered to start on that.
Can we make those separate issues that are referenced here? Aside from this being a large review already, there are major changes to the .module file that require quite a bit of manual steps to reroll.
- 🇬🇧United Kingdom longwave UK
Opened 📌 Discover cases where is no 1:1 map between field, prop and widget values Active to spin out the field widget discovery to a separate issue.
- 🇬🇧United Kingdom longwave UK
Let's get this refactoring + docs update in here and work on expanding the knowledge about field widgets in the followup issue.
A question about valid HTML but otherwise this looks good to me.
- Issue was unassigned.
- Status changed to Needs work
29 days ago 9:31pm 22 November 2024 - 🇺🇸United States effulgentsia
This looks really good and I'm ready to merge it, but it needs a reroll. Hopefully the last one :)
- 🇺🇸United States effulgentsia
Actually, looks like it didn't need a reroll, just merging from 0.x, but now tests are failing.
- First commit to issue fork.
- 🇫🇮Finland lauriii Finland
The MR was missing few use statements which led into the CI failures. The fix was simple enough so I went ahead and merged this with the fixes.
Automatically closed - issue fixed for 2 weeks with no activity.