-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: Add ElementRef
type to compat
#4557
base: main
Are you sure you want to change the base?
Conversation
export type ComponentPropsWithRef< | ||
C extends ComponentType<any> | keyof JSXInternal.IntrinsicElements | ||
> = C extends new ( | ||
export type ComponentPropsWithRef<C extends ElementType> = C extends new ( |
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.
We didn't originally have ElementType
when I added ComponentPropsWithRef
but someone kiindly added it a few months ago. It's worth switching this over, helps ensure things are kept in-sync.
For comparison: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b5dc32740d9b45d11cff9b025896dd333c795b39/types/react/index.d.ts#L1687-L1689
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.
How does this behave in 5.0?
That's a great question. Will investigate further |
Ignoring our resolution issues for the moment, this seems to work as we need? It does produce a type error in the compat declaration file, but in 5.0 & 5.1, the user is presented with a seemingly functional & identical type. This would require Copy/pasting the shadcn examples given in the issue, there doesn't seem to be a user-facing problem, it's just that the generic doesn't satisfy. I'm not well enough informed to say whether this is a problem or not though. |
I'm currently trying to chase down why so much of DT is broken right now due to preact; I can definitely say that you do not want to end up a situation where the types do not check without |
If you don't want to support TS 5.0, then theoretically we can bump all of the packages on DT up to a minimum of TS 5.1, though that may of course break someone. |
This has always been the case in Preact due to the limited aliasing mechanisms we have in TS; we don't provide every single React type, but do need consumers to use our types over As for issues on DefinitelyTyped we'd love to know more -- last release we did a fair few type changes (longstanding issues) and while we were a bit worried about doing it, we found no issues ourselves. Is it perhaps the added strictness in per-element types (#4546) or related to |
Re: We've held out on merging this due to that type error, as it seems TS won't use |
See: https://github.com/DefinitelyTyped/DefinitelyTyped/actions/runs/12009667461 Most failures here are due to those changes, it seems. |
typesVersions is a little annoying to use since it's largely an external config, not an internal one. The node types have full checking but it's nontrivial. It may be easier to just drop older versions |
That's unfortunate. Last time we bumped TS versions a lot of external projects had considerable issues, especially within the Angular ecosystem which I believe locks TS versions per-release. Preact, being a rather great option for widgets, sees a fair number of wrappers created around Preact components for use elsewhere. As such, I'm not a fan of touching that minimum, especially to raise it to something so new. |
The minimum version supported by DT is 5.0; 5.1 is not that far off I wouldn't think. But, I haven't dug more deeply into the problems here (but plan to do DT is unblocked) |
As for the DefinitelyTyped errors, I'm not sure what we're doing wrong here? That error would be "expected" if a TS 5.0 (or older) consumer was pulling our new types, but that shouldn't be happening to my knowledge. Our config should hopefully be directing those consumers to slightly different types: Lines 12 to 16 in bd14437
Lines 18 to 21 in bd14437
As for what versions DT supports, that doesn't really impact us here. We'd prefer support much further back than just 5.0 for our own repos I think. |
Hey @jakebailey, Thanks for dropping by! I am a bit confused how we broke this many tests in I've been looking into a few of these, I saw ember passing by which is definitely a React dependent. When we made the change to have variable support for < 5.1 and >= 5.1 we took this from As seen here - we have taken this PR as inspiration for #4548. Now I didn't feel 100% confident in the whole Dropping support for TS < 5.1 is a no-go, we have had grievances with this in the past with this as mentioned by Ryan and I would prefer for us not to do this again. We've been stable for a long time and doing breaking changes due to TypeScript in patch/minor versions is something I'd avoid as a service to our community. What can we do to help you here? |
I was busy fixing other DT issues and didn't get to looking at this one; I plan to look harder at this come tomorrow.
By nature of DT itself dropping 5.0, it's very possible that 5.0 will break for someone in the ecosystem anyway (via other types packages), but I'm sure we'll be able to find something that works anyway. Two years of types support is pretty generous when the alternative for downstream projects is to... Not update their deps? (Surely if they can update preact, they could update a devDep like TypeScript, but, I digress) |
I overcounted; the preact related failures are just:
You can see that the latter two ones are resolving
And indeed The I'm not sure if this PR is the right place to chat about this, happy to talk wherever. |
The
Ah damn, I missed that. If using
Wherever easiest/most comfortable for you, we appreciate you looking into it & helping us with this! Separate issue, discussion, we have a slack, or just keep everything here, wherever. |
Regarding DefinitelyTyped/DefinitelyTyped#65126 and #4548, note that the former duplicated the entire package, not just one file. Doing the split by file is more challenging, and only works if you can carefully control the way files are loaded. We were able to do this for a recent Node change, for example, but their types are global so it's not super hard to get right given everything starts from the root In your case, the solution may be to just not use relative paths in these handwritten files, which should just work? There aren't many files to double check. Otherwise, I would really only suggest duplicating everything as part of some build step, or maybe, introducing a separate compat package If you have a change, I'm super happy to check it locally. You can also clone DT and then point |
Okay so I think #4576 should correct our types here. As you suggested, it goes through and swaps out all references to the now-split types with a bare Hopefully that gets DefinitelyTyped working again. |
#4576 does seem to work to fix things. mdx is also fixed after the datetime thing, though needing a test change to account for I've commented on that PR, though, since it seems like the absolute import is not working out. |
Ah yeah, I think that should be the only intentional change now that we're offering stricter types. Glad that PR otherwise fixes things though! |
Closes #4481
For comparison: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/b5dc32740d9b45d11cff9b025896dd333c795b39/types/react/index.d.ts#L221-L235
Can confirm #4548 cleared the way for this -- like a total doofus I created a new vite project and was wildy confused to see the same old error re: can't assign
null | undefined
toVNode<any>
as I had only copied over this particular change. Eventually the issue dawned on me, copied over the new types, and it worked like a treat!