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

TypeScript errors after installing 7.2.3 relating to @types/react-redux now being a "dependency" #1700

Closed
marchaos opened this issue Mar 23, 2021 · 14 comments

Comments

@marchaos
Copy link

marchaos commented Mar 23, 2021

After installing 7.2.3, our build failed due to TS error pertaining to duplicate declarations. (see below). We've seen this before and I tracked it down to the release of react-redux last night. We initially had @types/react-redux in our package.json, which has since been removed, but the errors were still present. Due of the sub dependency of @types/react being 17.0.3 in react-redux,
and our explicit @types/react being 16.14.5, @types/react is not deduped. If we upgrade to 17.0.3 as well, then we no longer get errors, but this is forcing us to have a miss-match between our react version and the @types/react version.

It's questionable as to whether 7.2.3 supports react 16 because of this. Maybe this is a known issue and the action described above is what you'd expect?

TS Errors for reference for others:

node_modules/@types/react-redux/node_modules/@types/react/index.d.ts:2976:14 - error TS2300: Duplicate identifier 'LibraryManagedAttributes'.

2976         type LibraryManagedAttributes<C, P> = C extends React.MemoExoticComponent<infer T> | React.LazyExoticComponent<infer T>
                  ~~~~~~~~~~~~~~~~~~~~~~~~

  node_modules/@types/react/index.d.ts:2977:14
    2977         type LibraryManagedAttributes<C, P> = C extends React.MemoExoticComponent<infer T> | React.LazyExoticComponent<infer T>
                      ~~~~~~~~~~~~~~~~~~~~~~~~
    'LibraryManagedAttributes' was also declared here.

node_modules/@types/react-redux/node_modules/@types/react/index.d.ts:2987:13 - error TS2717: Subsequent property declarations must have the same type.
@markerikson
Copy link
Contributor

markerikson commented Mar 23, 2021

Hmm. That's a good point. I just grabbed the latest @types/react-redux when I made that change.

I'm not clear on how @types/react-redux and @types/react are versioned, and how those versions related to React versions.

I'm unfortunately busy with my day job atm. Can you do me a favor and put together a couple examples of how things behave with 7.2.3 and differing versions of React and the types?

@timdorr
Copy link
Member

timdorr commented Mar 23, 2021

Should the @types/ packages be peer deps instead of regular deps?

@markerikson
Copy link
Contributor

The intent of the change was to force that @types/react-redux gets installed automatically, so that users don't ever forget to add it. I know NPM 7 does something different with peer deps now, but NPM 6 and Yarn don't install them.

If this turns out to have been a really bad idea we can definitely backtrack, but I'd like to get more info on exactly what scenarios are busted here.

@marchaos
Copy link
Author

@markerikson From what I can see, the latest 16.x and 17.x of the @types/react and @types/react-redux are compatible (i.e no TS errors) and similarly, react 16 and and 7.2.3 react-redux are ok too (besides the TS errors). Our main blocker for upgrading to react 17 is enzyme not being compatible, so for now, we've gone back to 7.2.2.

@chrisbutler
Copy link

we are in the same situation as @marchaos. i was also surprised to find the types inclusion in a minor version bump, given that it causes TS errors and has the potential to break build processes

@markerikson
Copy link
Contributor

Two quick thoughts:

  • Given how TS works and its own lack of semantic versioning, seemingly any change to anything types related could hypothetically cause breakage somewhere
  • I made the change to auto-depend on the types because we've repeatedly seen people forget to install them, so I figured this would simplify things for most people. First I knew that there were any potential problems with that approach was seeing this issue filed.

So far no one has provided additional information indicating what a specific problem might be with regards to types versions compatibility, and my focus has been finalizing our RTK Query package and merging it into Redux Toolkit. So, no, it hasn't been on my radar.

If this is still an active problem for people, I need more info about what sorts of actual interactions cause these errors - combinations of package versions, etc.

@julienw
Copy link

julienw commented May 3, 2021

I'd like to add that @types/react-redux depends itself on redux, so this fails the goal of we've completely dropped the dependency on Redux as stated in the latest changelog.

@markerikson
Copy link
Contributor

Hmm. Hadn't thought about that part, but we did at least eliminate the use of Redux as a runtime dependency.

@julienw
Copy link

julienw commented May 4, 2021

Hmm. Hadn't thought about that part, but we did at least eliminate the use of Redux as a runtime dependency.

mmm, but did you really? :-)

The entry @types/react-redux is in dependencies not devDependencies see:

react-redux/package.json

Lines 49 to 56 in 86e962e

"dependencies": {
"@babel/runtime": "^7.12.1",
"@types/react-redux": "^7.1.16",
"hoist-non-react-statics": "^3.3.2",
"loose-envify": "^1.4.0",
"prop-types": "^15.7.2",
"react-is": "^16.13.1"
},

Similarly for the @types/react-redux types (probably right in this case):
https://github.com/DefinitelyTyped/DefinitelyTyped/blob/813a8799e465a7d5f0d6776643f20f93681e85e4/types/react-redux/package.json#L3-L7

(To be clear, I don't think this is a huge problem, after all redux is needed to use react-redux at one point -- but this may cause issues in the future if/when redux changes its major version).

@gugu
Copy link

gugu commented May 6, 2021

(To be clear, I don't think this is a huge problem, after all redux is needed to use react-redux at one point -- but this may cause issues in the future if/when redux changes its major version).

redux changed their minor version and now @types/react-redux installs redux@4.0.0 and my code installs redux@4.1.0. What do you think about including d.ts files in this repo instead? (I don't plan to describe what do I think about DefinitelyTyped monorepo because it definitely violates Code of Conduct). I can create a PR with these changes

@julienw
Copy link

julienw commented May 6, 2021

redux changed their minor version and now @types/react-redux installs redux@4.0.0 and my code installs redux@4.1.0.

This is true, but on my project, this results in installing redux 4.1 for both, as redux 4.1 fulfills redux@^4.0.0. But we're deduplicating systematically, so this might be different if you don't. (also that's why I mentioned the major version)

That said I think your proposal makes sense, especially if the maintainers of the project also maintain these types.

@gugu
Copy link

gugu commented May 6, 2021

#1717

@markerikson
Copy link
Contributor

Thing is, we don't maintain the React-Redux types ourselves. The community has always done so. That's why they lived in DefinitelyTyped instead of in this repo.

We did add myself and Lenz to the "list of maintainers for these types" in DTS recently because someone else had filed a PR that marked one of the types as deprecated incorrectly without ever consulting us about the intended usage, so we want to be notified in case anyone ever tries to make unwanted changes like that in the future. But, we haven't actually worked on the types ourselves.

When we get around to working on React-Redux v8, we'll rewrite the library in TS. Until then, the types can stay where they are.

@markerikson
Copy link
Contributor

Given that React-Redux v8 will be out shortly, and is written directly in TS, I think this can be closed as OBE.

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

No branches or pull requests

6 participants