-
Notifications
You must be signed in to change notification settings - Fork 558
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
fix: do not set width binding on Builder Column if value is undefined #1642
Conversation
🦋 Changeset detectedLatest commit: d8bc121 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 |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit d8bc121. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 4 targetsSent with 💌 from NxCloud. |
...(col.width !== undefined && { | ||
bindings: { | ||
width: { code: col.width.toString() }, | ||
}, | ||
}), |
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.
is it possible to move the undefined check to the expression for width
inside the bindings
?
...(col.width !== undefined && { | |
bindings: { | |
width: { code: col.width.toString() }, | |
}, | |
}), | |
bindings: { | |
...(col.width !== undefined && { width: { code: col.width.toString() } }), | |
}, |
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 should be possible, but I think that would result in an empty bindings: {}
if the width is defined. I tried to follow the style below that where don't add the properties
object if col.link
is undefined
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 seems more robust to me that we improve our JSX generator to not print bindings with undefined values, no?
mitosis/packages/core/src/generators/mitosis/generator.ts
Lines 138 to 140 in a76da9d
for (const key in json.bindings) { | |
const value = json.bindings[key]?.code as string; | |
wouldn't a if (!value) continue
in here be enough to handle this universally?
Sami and I discussed offline, and we need both my change and the one Sami suggested. That has been applied, so I'm going to merge this. |
Description
We always create a
width
binding even if the width is undefined. This causes issues when converting a Mitosis node to JSX as it will yield<Column width={} />
. Empty expressions (width={}
) are not valid in JSX, so this will crash and JSX parser.I fixed the issue to only add a width binding when the value is defined.
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.