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

Fix types for @types/react@18 #1798

Merged
merged 3 commits into from
Jan 28, 2023
Merged

Fix types for @types/react@18 #1798

merged 3 commits into from
Jan 28, 2023

Conversation

karlhorky
Copy link
Contributor

@karlhorky karlhorky commented Apr 24, 2022

Ref: #1062 (comment)
Ref: https://github.com/eps1lon/types-react-codemod#implicit-children
Ref: DefinitelyTyped/DefinitelyTyped#46691

Found some time to take a look at the breakages with @types/react@18, and opened a PR to fix the types in the @react-pdf/renderer package: #1798

However, this does not fix the other packages in the monorepo. And this may cause breakage for @types/react versions lower than 18.

cc @diegomura @jeetiss

Copying my comment here for visibility:

Hm, but it fails with @types/react v18, which was just published (because of the breaking changes):

Run yarn tsc
yarn run v1.22.18
$ /home/runner/work/project/node_modules/.bin/tsc
Error: project/generatePdf.tsx(212,6): error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: DocumentProps | Readonly<DocumentProps>): Document', gave the following error.
    Type '{ children: Element[]; }' has no properties in common with type 'IntrinsicAttributes & IntrinsicClassAttributes<Document> & Readonly<DocumentProps>'.
  Overload 2 of 2, '(props: DocumentProps, context: any): Document', gave the following error.
    Type '{ children: Element[]; }' has no properties in common with type 'IntrinsicAttributes & IntrinsicClassAttributes<Document> & Readonly<DocumentProps>'.
Error: project/generatePdf.tsx(213,8): error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: PageProps | Readonly<PageProps>): Page', gave the following error.
    Type '{ children: Element; size: "A4"; style: { fontFamily: string; fontSize: number; lineHeight: number; paddingTop: number; paddingBottom: number; paddingHorizontal: number; }; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
  Overload 2 of 2, '(props: PageProps, context: any): Page', gave the following error.
    Type '{ children: Element; size: "A4"; style: { fontFamily: string; fontSize: number; lineHeight: number; paddingTop: number; paddingBottom: number; paddingHorizontal: number; }; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
Error: packages/learn.upleveled.io-server/routes/studentAssessments/learningOutcomesPdf.tsx(259,12): error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: SVGProps | Readonly<SVGProps>): Svg', gave the following error.
    Type '{ children: Element[]; style: { marginTop: number; marginBottom: number; }; width: string; height: string; viewBox: string; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Svg> & Readonly<SVGProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Svg> & Readonly<SVGProps>'.
  Overload 2 of 2, '(props: SVGProps, context: any): Svg', gave the following error.
    Type '{ children: Element[]; style: { marginTop: number; marginBottom: number; }; width: string; height: string; viewBox: string; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Svg> & Readonly<SVGProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Svg> & Readonly<SVGProps>'.
Error: project/generatePdf.tsx(266,16): error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: LinearGradientProps | Readonly<LinearGradientProps>): LinearGradient', gave the following error.
    Type '{ children: Element[]; id: string; x1: string; x2: string; y1: string; y2: string; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<LinearGradient> & Readonly<LinearGradientProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<LinearGradient> & Readonly<LinearGradientProps>'.
  Overload 2 of 2, '(props: LinearGradientProps, context: any): LinearGradient', gave the following error.
    Type '{ children: Element[]; id: string; x1: string; x2: string; y1: string; y2: string; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<LinearGradient> & Readonly<LinearGradientProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<LinearGradient> & Readonly<LinearGradientProps>'.
Error: project/generatePdf.tsx(285,8): error TS2769: No overload matches this call.
  Overload 1 of 2, '(props: PageProps | Readonly<PageProps>): Page', gave the following error.
    Type '{ children: Element; size: "A4"; style: { fontFamily: string; fontSize: number; lineHeight: number; paddingTop: number; paddingBottom: number; paddingHorizontal: number; }; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
  Overload 2 of 2, '(props: PageProps, context: any): Page', gave the following error.
    Type '{ children: Element; size: "A4"; style: { fontFamily: string; fontSize: number; lineHeight: number; paddingTop: number; paddingBottom: number; paddingHorizontal: number; }; }' is not assignable to type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
      Property 'children' does not exist on type 'IntrinsicAttributes & IntrinsicClassAttributes<Page> & Readonly<PageProps>'.
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Error: Process completed with exit code 2.

@karlhorky karlhorky mentioned this pull request Apr 24, 2022
@eizgg
Copy link

eizgg commented May 3, 2022

Hi guys, any update?

Copy link

@guillaume-acard guillaume-acard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested with the changes and works fine 👍

@leeroyrose
Copy link

Hey any updates on when this will get released?

@karlhorky
Copy link
Contributor Author

@leeroyrose You'll have to ask @diegomura and @jeetiss, I don't have commit / publishing access.

@ghost
Copy link

ghost commented Aug 19, 2022

Any update on getting this merged?

@karlhorky
Copy link
Contributor Author

@diegomura @jeetiss I saw that you released a new version @react-pdf/renderer 3.0.0. I guess the major version bump would have unfortunately been a good time to get this change in, in case it is a breaking change for anyone.

But would you do this in a minor instead?

@antoineharel
Copy link

Hey @diegomura , this seems good, any plan to merge it ?

@BarrieLAJ
Copy link

Please could you release this update because we are trying to use the SVG component but without the children props in the types we cannot build the project.

@antoineharel
Copy link

antoineharel commented Sep 27, 2022

I ended up creating custom components until an official update is released.
PropsWithChildren does the job here:

import ReactPDF from '@react-pdf/renderer';
import { FC, PropsWithChildren } from 'react';

export const Svg: FC<PropsWithChildren<ReactPDF.SVGProps>> = ({ ...props }) => (
  <ReactPDF.Svg {...props} />
);
export const G: FC<PropsWithChildren<ReactPDF.GProps>> = ({ ...props }) => (
  <ReactPDF.G {...props} />
);
export const ClipPath: FC<PropsWithChildren<ReactPDF.ClipPathProps>> = ({
  ...props
}) => <ReactPDF.ClipPath {...props} />;

@BarrieLAJ
Copy link

I ended up creating custom components while an official update is released. PropsWithChildren does the job here:

import ReactPDF from '@react-pdf/renderer';
import { FC, PropsWithChildren } from 'react';

export const Svg: FC<PropsWithChildren<ReactPDF.SVGProps>> = ({ ...props }) => (
  <ReactPDF.Svg {...props} />
);
export const G: FC<PropsWithChildren<ReactPDF.GProps>> = ({ ...props }) => (
  <ReactPDF.G {...props} />
);
export const ClipPath: FC<PropsWithChildren<ReactPDF.ClipPathProps>> = ({
  ...props
}) => <ReactPDF.ClipPath {...props} />;

Thanks very much this has worked for me.

@yangricardo
Copy link

I ended up creating custom components while an official update is released. PropsWithChildren does the job here:

import ReactPDF from '@react-pdf/renderer';
import { FC, PropsWithChildren } from 'react';

export const Svg: FC<PropsWithChildren<ReactPDF.SVGProps>> = ({ ...props }) => (
  <ReactPDF.Svg {...props} />
);
export const G: FC<PropsWithChildren<ReactPDF.GProps>> = ({ ...props }) => (
  <ReactPDF.G {...props} />
);
export const ClipPath: FC<PropsWithChildren<ReactPDF.ClipPathProps>> = ({
  ...props
}) => <ReactPDF.ClipPath {...props} />;

That really solved, thanks!!

@vieirai
Copy link

vieirai commented Oct 17, 2022

Any movement on this?
The patch above, is nice and works, but would be nice to have it working properly without having to redefine the Components.

@Jonur
Copy link

Jonur commented Oct 17, 2022

Hi all, have we got any news on when this will be merged?

@diegomura
Copy link
Owner

Hi all. Sorry for the lack of response in here. Been very busy and also on vacations 😄 Code looks good, but this would break support for versions prior 18 as far as I can tell. I'm open to suggestions, but I would like to prevent that as much as possible.

@ghost
Copy link

ghost commented Nov 7, 2022

This is really issue with all of my projects and using ts-ignore in someplace to overcome these. Most of project are using recent version of react. Better to support the latest with new react-pdf versions

@diegomura
Copy link
Owner

@chathu-novade isn't that a bit subjective? Not all projects can afford upgrading react so easily. So bumping react-pdf to 18 without being backwards compatible will break it to many. Merging this will just mean switching the sub-group of users to which this lib will be broken

@ghost
Copy link

ghost commented Nov 7, 2022

I mean we can release with a major version with react 18 support. Like many other react related projects does. I'm open to other suggestions. There are lot of issues created asking for 18 support.

@ghost
Copy link

ghost commented Nov 7, 2022

For this PR I think react types needs to be updated to use this. Which also currently we have done since we had issues with old types with react-pdf/renderer with Typescript.

@karlhorky
Copy link
Contributor Author

karlhorky commented Nov 7, 2022

I mean we can release with a major version with react 18 support

Yeah I would also suggest this. Often it's not reasonable to support all old versions of dependencies in a backwards-compatible way.

I've seen other major open source libraries also take this approach of only supporting React 18+ in the new versions.

@diegomura
Copy link
Owner

Thanks for the feedback! @karlhorky can you share what those libs are? I'm curious how they handled that. This project is in the most part very React agnostic (except for the reconciler and types) so dropping support just to have those merged seems a bit overkill. I wonder if there's a way to (via a postinstall script maybe) toggle deps based on the targeted react version

@karlhorky
Copy link
Contributor Author

karlhorky commented Nov 7, 2022

Can't remember the other ones off the top of my head, but considering how large of a release v18 is, I would imagine there will be more than just one.

Another way of going about it (if backwards compatibility is very important to you) is that you could maintain two versions - @react-pdf/[email protected] for pre-v18 versions, @react-pdf/renderer@>=4.0.0 for v18+.

Maintaining conditional types / scripts seems difficult to deal with / buggy to me, but that would also be an option.

@diegomura
Copy link
Owner

Thanks @karlhorky ! Both cases I think are still maintaining old versions, at least until a certain window. Backwards compatibility is important, as I know for certain some projects and companies using this lib won't have react migration to easy to do. Maintaining 2 parallel versions is a good approach, but this project does not have the dev power gatsby or next has, that why I was curious how I can prevent that.

I'll research about prepare or postinstall scripts. Another alternative would be extracting everything that's React version specific (which is not much) to an internal package, and require users to install a specific version of it depending what react version they are targeting. We can default to latest React version always. This is technically branching versions, but in a smaller codebase

@karlhorky
Copy link
Contributor Author

karlhorky commented Nov 7, 2022

Another alternative would be extracting everything that's React version specific (which is not much) to an internal package, and require users to install a specific version of it depending what react version they are targeting. We can default to latest React version always. This is technically branching versions, but in a smaller codebase

Oh interesting, that may be nicer / less moving parts than postinstall scripts or similar 👍 I've had some bad times with trying to get postinstall scripts to work reliably...

@gitname gitname mentioned this pull request Dec 16, 2022
@karlhorky
Copy link
Contributor Author

karlhorky commented Jan 28, 2023

Thanks for the merge! I'll give it a try in @react-pdf/[email protected].

@dimitur2204
Copy link

Solved a lot of problems!

@jeetiss
Copy link
Collaborator

jeetiss commented Jan 28, 2023

@dimitur2204 thanks, i hope so

@karlhorky, try @react-pdf/[email protected] because 3.1.0 is broken 🥲

@karlhorky
Copy link
Contributor Author

Tried out @react-pdf/[email protected] - it seems to be working, thanks!

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

Successfully merging this pull request may close these issues.