-
Notifications
You must be signed in to change notification settings - Fork 616
fix: Builder Text node is preserved when converting to Mitosis JSON #1769
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 8e20873 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
View your CI Pipeline Execution ↗ for commit 8e20873.
☁️ Nx Cloud last updated this comment at |
@@ -90,10 +90,7 @@ | |||
} | |||
}, | |||
"component": { | |||
"name": "Text", | |||
"options": { | |||
"text": "Enter some text..." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example has both component.options.text
as well as bindings.component.options.text
. Leaving both in created a behavior change with this PR. I spoke with Sami and he said that this use case is a bit unusual so we decided to not worry about supporting it for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I'll suggest a review from @samijaber / @sidmohanty11 too.
@@ -2730,89 +2743,89 @@ export default function MyComponent(props) { | |||
<div> | |||
<Fragment> | |||
<Show when={props.conditionA}> | |||
<span>Content0</span> | |||
<Text $tagName=\\"span\\" text=\\"Content0\\" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❔question:
- The
span
/div
tag, for gen2 sdks, would be transformed to the framework specific element right ? So with this change we would need update the corresponding tests / snapshots in the gen 2 sdks repo right ? - Since there are only changes in the snapshots for
builderContentToMitosisComponent
tests and there are no snapshot changes forcomponentToBuilder
tests, I'm assuming we won't have any breaking change in Builder JSON with regards to how mitosis is being used within AI services. Just wanted to confirm that.
\\"builder-id\\": \\"builder-192569f8d0a943398ec7ab9c327e104f\\", | ||
class: \\"cjrqfb1 builder-block\\", | ||
style: { | ||
backgroundColor: (() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we lost bindings (class, style, "builder-id", key
in another one below) in these tests. Do you know why? A bit concerning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! This was a bug in my compile away implementation. Qwik does not render any bindings it sees on a node with _text
(since it's supposed to be plain text).
I fixed the implementation by making sure bindings/props (except for _text) are rendered on the parent div: 1c7eb2e
However, this did uncover an inconsistency in the Qwik generator:
When a node has both an innerHTML binding and property, we prefer the binding. However, when the node has a _text
binding and property we prefer the property.
@@ -3569,32 +3579,35 @@ exports[`Builder > bindings 1`] = ` | |||
"bindings": { | |||
"css": { | |||
"bindingType": "expression", | |||
"code": "{backgroundColor:'rgba(0, 0, 0, 1)',display:'flex',paddingLeft:'72px',paddingRight:'72px',paddingTop:'25px',paddingBottom:'25px',alignItems:'start',gap:'10px',fontFamily:'Poppins, -apple-system, Roboto, Helvetica, sans-serif',fontSize:'16px',color:'rgba(255, 255, 255, 1)',fontWeight:'700',textTransform:'uppercase',justifyContent:'start'}", | |||
"code": "{\\"backgroundColor\\":\\"rgba(0, 0, 0, 1)\\",\\"display\\":\\"flex\\",\\"paddingLeft\\":\\"72px\\",\\"paddingRight\\":\\"72px\\",\\"paddingTop\\":\\"25px\\",\\"paddingBottom\\":\\"25px\\",\\"alignItems\\":\\"start\\",\\"gap\\":\\"10px\\",\\"fontFamily\\":\\"Poppins, -apple-system, Roboto, Helvetica, sans-serif\\",\\"fontSize\\":\\"16px\\",\\"color\\":\\"rgba(255, 255, 255, 1)\\",\\"fontWeight\\":\\"700\\",\\"textTransform\\":\\"uppercase\\",\\"justifyContent\\":\\"start\\"}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
im seeing css code expressions in snapshots across this PR have their formatting updated from single quotes to escaped double-quotes. it looks like purely a formatting change, but i'm still surprised why it happens. Do you know? Is it a symptom of a potential problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference comes down to our use of both JSON.stringify
and json5.stringify
. The former adds double quotes, the latter does not.
More specifically, in main
if we find a node with a single child Text node, we will try to merge code which uses json5.stringify
for the CSS. This code was removed in this PR, so json5.stringify
is never called and instead the original JSON.stringify
formatting is preserved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we don't have that code path anymore because by merging the code we compile away the Text node which we are trying to avoid.
Problem
Builder Text nodes are compiled away by default when converting to Mitosis JSON (and by extension Mitosis JSX). This means that
<Text text="foo" />
becomes<div>foo</div>
. When converting Mitosis JSX back to Builder JSON we intentionally do not convert the div back to a Builder Text node because we have no idea if it's supposed to remain as a regular div.This causes issues because the Builder Text primitive has additional styling built-in. When we convert JSX back to Builder JSON that styling is effectively lost since the Text node is not re-created. This can result in rendering differences due to the loss of the Text node.
Solution
Converting from Builder JSON to Mitosis JSON no longer compiles away Builder Text nodes by default. As a result,
<Text text="foo" />
remains as-is when converting to Mitosis JSON/JSX. Similarly, the Text primitive is preserved when converting back to Builder JSON.The
compileAwayBuilderComponents
function has been updated to account forText
, so developers who still wish to have this primitive compiled away can use that function to do so.Other Information
compileAwayBuilderComponents
simple, but we can add logic there as needed.Text
primitive isn't handled properly. To avoid playing "whack a mole" and making this PR larger than it already is, I left TODOs where necessary.Make sure to follow the PR preparation steps in CONTRIBUTING.md before submitting your PR:
yarn fmt:prettier
.yarn test:update
yarn g:changeset
and follow the CLI instructions. Alternatively, use the Changeset Github Bot to create the file.