Skip to content
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: Unable to return ComponentChild from component #4113

Closed
1 task done
rschristian opened this issue Aug 23, 2023 · 4 comments · Fixed by #4548
Closed
1 task done

Types: Unable to return ComponentChild from component #4113

rschristian opened this issue Aug 23, 2023 · 4 comments · Fixed by #4548
Labels

Comments

@rschristian
Copy link
Member

  • Check if updating to the latest Preact version resolves the issue

Describe the bug

From Slack

To Reproduce

TS Playground Link

The React equivalent, using ReactNode, works just fine. TS Playground Link

I believe we need to change this line to return a ComponentChild as React does but this seems to break a whole bunch of our types.

@augustjk
Copy link

augustjk commented Sep 1, 2023

@rschristian The change you proposed of updating return type of FunctionComponent to include ComponentChild does work but only on TS >= 5.1 due to this change https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-1.html#decoupled-type-checking-between-jsx-elements-and-jsx-tag-types

What's considered a valid JSX is now based on the tag specifier satisfying JSX.ElementType instead of its return types satisfying JSX.Element. In TS versions 5.0 and lower, JSX.Element type for preact is still VNode<any> so the return type of FunctionComponent changing to ComponentChild is too broad.

@types/react addresses this by forking the type so that users of TS <= 5.0 still get the old type.
PR that created the fork and added package type export mapping: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/65126/files
The index.d.ts in ts5.0 still has return type ReactElement: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/05094b6df4fb6f4404333725f13d63dd8aabeb34/types/react/ts5.0/index.d.ts#L532

@rschristian
Copy link
Member Author

rschristian commented Sep 1, 2023

Interesting, thank you! I had no clue what was going so wrong there.

If that's the approach that's needed for now, I wonder if it's better to wait... that's rather, rough, and I doubt our tooling here is going to support that nicely. I think forking the types for different TS versions is a non-starter.

@augustjk
Copy link

augustjk commented Sep 1, 2023

Ah this appears to have been visited before: #3611 It was just ahead of its time.

Is forking the type so bad? It's unfortunate that TS only recently made it possible for function components to return a broader type like this, but as it is, the typing is incorrect.

Without forking, I suppose you'd need to decide when to update the version of typescript for this repo and make the change to the FunctionComponent return type then. Users would also be forced to update their TS version if they want to use the new type, and users that don't want to update TS are stuck with the old version.

@rschristian
Copy link
Member Author

Is forking the type so bad?

It creates quite the mess and looks exceptionally fragile/easy for things to be out of sync, so I'd be against it.

I think we should just stick with the types as-is for the time being, we definitely don't want to bump TS versions (recently that caused quite a bit of problems for people using Preact component libraries in older Angular projects, where I guess TS version is locked in?).

This seems like a pretty uncommon issue to run across and can always be solved with a wrapping Fragment or something anyhow, but that's just my 2¢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants