-
Notifications
You must be signed in to change notification settings - Fork 6
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
Add Solid JS support #24
Conversation
🦋 Changeset detectedLatest commit: 1a6ee1e 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 |
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.
nit on the use of Show
, but this is a pretty solid PR 👏
} | ||
|
||
return ( | ||
<Show when={fieldState()}> |
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.
I'm not sure we need this Show
check if we're throwing above? We could also remove the function wrapper on fieldState
without this check I think
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.
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.
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.
Do you have other ideas? I could change the tsconfig for this template to strict instead of strictest and it will work in this approach
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.
Ahh thanks, I see the issue now. I'd like to keep strictest
since this will be generated code in the user's project, and I want to pass the strictest config possible to ensure we don't raise type errors in your project.
I figured out the problem though! We need to hoist the state inside the state function. That way it can guarantee the function never returns undefined
, since it's always mapped to an error. The solution you showed above does not guarantee that. You can check fieldState() !== undefined
, but what if calling that function a second time returns a different value? TypeScript unfortunately isn't smart enough to see that.
sanitizeFormResult | ||
].forEach((formResult) => { | ||
if (formResult?.data) { | ||
console.log(formResult.data); |
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.
Nice refactor!
Changes