-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
types: improve the types returned by styled #32
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
test is failing due to some issue related to corepack and the npm registry I could find some workaround right now but I'd rather just wait for that to settle a bit EDIT: all good 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.
Thank you so much for fixing these types up and adding tests 🙏 just a question about the package manager change, but otherwise this looks great!
@@ -6,4 +6,4 @@ | |||
dist | |||
node_modules | |||
out | |||
|
|||
tsconfig.vitest-temp.json |
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 haven't seen this before, does this get added from --typecheck
?
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.
yeah, it's a temporary file that only exists while tests are running. as soon as the tests are finished it will be deleted.
I've added it here mainly so it doesn't show up in git commands or the vscode ui
"vitest-browser-react": "^0.0.4" | ||
}, | ||
"engines": { | ||
"node": ">=20.0.0" | ||
}, | ||
"packageManager": "[email protected]" | ||
"packageManager": "[email protected]+sha512.c89847b0667ddab50396bbbd008a2a43cf3b581efd59cf5d9aa8923ea1fb4b8106c041d540d08acb095037594d73ebc51e1ec89ee40c88b30b8a66c0fae0ac1b", |
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 there a reason we need to pin to a sha?
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 works either way. corepack adds it by default because it makes installation less prone to supply chain attacks if e.g. a registry server was compromised. on the other hand, it's a bit annoying to have a massive hash in the package file.
if you prefer without lmk and I'll rebase it out of existence (or you can)
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.
Ah, didn't realize corepack did that by default, makes sense! Yeah, I can update it and keep a simple version for now.
improvements, in no particular order:
if we were to use a style prop that already existed on the component, our type would not reflect it
in this example, previously
onClick
wasMouseEventHandler<HTMLDivElement> & string
, but because style props are filtered, a more correct type is juststring
because style props are filtered, if a required component prop and a style prop share a name we now get a type error
previously, if style props was a function our component props were expanded to
Record<string, unknown>
which would let us add incorrect propsthis also adds support for generic type propagation, so if we style a generic function component the generic will be preserved. do note that generic class components will not be propagated, only generic function components.