supasate / connected-react-router

A Redux binding for React Router v4
MIT License
4.73k stars 592 forks source link

Provide an update to work with `@types/react@18` #565

Closed txemaleon closed 2 years ago

txemaleon commented 2 years ago

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch connected-react-router@6.9.2 for the project I'm working on.

In the company I'm working on recently updated to react 18 which no longer includes the children property for react components.

This causes our CI Pipeline to blow up when checking types and makes our developer experience worse.

We had to update most of our type packages and some other packages with types included, but we were unable to update connected-react-router since it's already in the last version.

We used patch-package to fix the problem really easily and we wanted to share the solution so you can apply the fix easily (or anyone who runs into this problem).

Here is the diff that solved my problem:

diff --git a/node_modules/connected-react-router/index.d.ts b/node_modules/connected-react-router/index.d.ts
index f2e7bcc..79ed1ff 100644
--- a/node_modules/connected-react-router/index.d.ts
+++ b/node_modules/connected-react-router/index.d.ts
@@ -17,6 +17,7 @@ declare module 'connected-react-router' {
   type PathParam = Parameters<typeof matchPath>[1];

   interface ConnectedRouterProps<S = LocationState> {
+    children: React.ReactNode;
     history: History<S>;
     context?: React.Context<ReactReduxContextValue>;
     noInitialPop?: boolean;

This issue body was partially generated by patch-package.

Cheers.

laurentksh commented 2 years ago

@supasate hey, would be nice to have an official fix for this issue (I can create a PR for it if you want)

devon-whil commented 2 years ago

See the following comment which has some helpful links: https://github.com/facebook/react/issues/24304#issuecomment-1111559894

You could argue that the problem is with every package that specifies version * as a dependency.

The longer-term solution to this is being discussed in https://github.com/microsoft/DefinitelyTyped-tools/issues/433.

The short-term workarounds are described in https://github.com/facebook/react/issues/24304#issuecomment-1094565891.

txemaleon commented 2 years ago

We took the fast route and added a @ts-ignore where we use this component and continued to work.

At some point we'll get rid of this package since it isn't maintained anymore.

antonpegov commented 2 years ago

@txemaleon do you mean connected-react-router isn't maintained anymore?

txemaleon commented 2 years ago

I'd say so based on the issues list and the lack of responses.