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

[Bug?]: Yarn 2 PnP causes incorrect Typescript typings with redux-toolkit #3150

Closed
1 task done
joekrill opened this issue Jul 21, 2021 · 6 comments · Fixed by #3152
Closed
1 task done

[Bug?]: Yarn 2 PnP causes incorrect Typescript typings with redux-toolkit #3150

joekrill opened this issue Jul 21, 2021 · 6 comments · Fixed by #3152
Labels
bug Something isn't working

Comments

@joekrill
Copy link

joekrill commented Jul 21, 2021

Self-service

  • I'd be willing to implement a fix

Describe the bug

Upgrading to Yarn 2 with PnP in a typescript project that uses redux-toolkit causes the type that configureStore generates to be incorrect.

When using redux-toolkit with typescript, the documentation describes how to type the application Dispatch type: https://redux-toolkit.js.org/usage/usage-with-typescript#getting-the-dispatch-type

Ultimately, the code is something like this:

import { configureStore } from '@reduxjs/toolkit';
import { useDispatch } from 'react-redux';
import rootReducer from './rootReducer';

const store = configureStore({  reducer: rootReducer });

export type AppDispatch = typeof store.dispatch;
export const useAppDispatch = () => useDispatch<AppDispatch>();
export default store;

Which results in the AppDispatch being typed in a specific way. In particular, it is typed so that it knows it can accept "thunks". It ends up looking something like this (with the key part being that it includes a ThunkDispatch typing):

Screen Shot 2021-07-20 at 9 33 41 PM

After upgrading to Yarn 2 and enabling PnP, the types are significantly different and incorrect, with the ThunkDispatch type missing (among other things):

Screen Shot 2021-07-20 at 9 53 04 PM

This isn't just limited to VSCode -- running a typecheck from the command line (yarn run tsc --noEmit) produces errors caused by this incorrect typing.

I really don't know if this is a bug in Yarn, or perhaps an issue with how Yarn 2 patches typescript, or maybe it's even a typing issue with redux-toolkit (or a dependency). But I'm able to reproduce the issue using a working repo using Yarn 1, upgrading it to Yarn 2, and enabling PnP. Everything works as expected until the point of switching to PnP. I'm not even sure how to debug this further, but so far what I've gathered points to it being an issue with PnP. I was hoping someone here might have some additional insight.

To reproduce

To reproduce, I started with this basic "starter-kit" repo that uses Yarn 1: https://github.com/davidjamescarter/react-redux-thunk-typescript-redux-toolkit-starter-kit

I forked that and migrated it to Yarn 2: https://github.com/joekrill/react-redux-thunk-typescript-redux-toolkit-starter-kit

There are 2 additional commits: the first migrates to Yarn 2 without PnP. The final commit enabled PnP. And that's when the typing problems occur.

Environment

System:
OS: macOS 11.4
CPU: (8) x64 Apple M1
Binaries:
Node: 14.16.0 - /private/var/folders/g0/j5y10l9s7dq8q2qqm9r92xl00000gn/T/xfs-44942072/node
Yarn: 2.4.2 - /private/var/folders/g0/j5y10l9s7dq8q2qqm9r92xl00000gn/T/xfs-44942072/yarn
npm: 6.14.11 - ~/.nvm/versions/node/v14.16.0/bin/npm

Additional context

No response

@joekrill joekrill added the bug Something isn't working label Jul 21, 2021
@joekrill
Copy link
Author

I was able to get this working in the test project by upgrading the @reduxjs/toolkit package to the latest version and adding redux as an explicit dependency: https://github.com/joekrill/react-redux-thunk-typescript-redux-toolkit-starter-kit/tree/working

However, I have another project where that did NOT fix the problem. That project uses workspaces. So I converted the test project to use workspaces, and the problem reappears: https://github.com/joekrill/react-redux-thunk-typescript-redux-toolkit-starter-kit/tree/monorep. BUT! - if I explicitly add redux as a dependency to the workspace root, the problem is resolved (and that works in both the test project and my other project using workspaces). So at least there's a workaround for the time being (although obviously it's not ideal to have to add that dependency to the workspace root).

So I think this has something to do with typescript not loading the types for redux in certain cases? But again, this only happens using Yarn 2 PnP - all other scenarios I've tried work correctly. So I suspect this has something to do with the way typescript is being patched? It's also odd that it's the redux package that has to be added explicitly - and not redux-thunk - which is the package that has the Thunk-related types defined.

And to be thorough, here are some additional things I've also tried with no success:

  1. Upgrade all dependencies to latest versions
  2. Addressing all YN0002 errors using the packageExtensions option in .yarnrc.yml
  3. Downgrading to previous versions of Yarn 2 - this caused an error where Yarn couldn't patch typescript correctly (some sort of "could not apply hunk" error) - I'm guessing I'd need to downgrade typescript, too?
  4. Upgrading to Yarn 3.0.0-rc.12
  5. Unplugging the various combinations of typescript, redux, redux-thunk, and @reduxjs/toolkit
  6. yarn dedupe

@arcanis
Copy link
Member

arcanis commented Jul 21, 2021

Generally when this happen it's because the type package doesn't declare its dependency on whatever package it depends on for its type (even if its runtime doesn't depend on it itself). TypeScript then silently fallbacks to any. It gives the impression of working under older install strategies since hoisting can hide such issues.

To debug that, I'd recommend to use ctrl+click to jump into the redux toolkit types, then see what are the imports in this file. Once you find the one that becomes any, check if the package declares it as a dependency.

@merceyz
Copy link
Member

merceyz commented Jul 21, 2021

The issue is that redux-thunk doesn't declare redux as a peer dependency which can be fixed with the following extension.

packageExtensions:
  'redux-thunk@*':
    peerDependencies:
      redux: '*'

Interestingly it was fixed two years ago but it hasn't been released it yet, I'll merge it into our builtin packageExtensions so it doesn't happen again reduxjs/redux-thunk#251

The second error is that you're missing types for @testing-library/react which is fixed by running

yarn add @types/testing-library__react@^9.0.0

The way I debugged this was to run PnP in debug mode so it prints out all rejected resolutions

PNP_DEBUG_LEVEL=1 yarn tsc

That gives a lot of output but the important part is

✖ redux-thunk tried to access redux, but it isn't declared in its dependencies; this makes the require call ambiguous and unsound.

  In ← 'redux'
  In ← 'C:/berry/cache/redux-thunk-npm-2.3.0-a5eb89e35e-8.zip/node_modules/redux-thunk/'
  In ← { considerBuiltins: false }

However, I have another project where that did NOT fix the problem. That project uses workspaces. So I converted the test project to use workspaces, and the problem reappears: https://github.com/joekrill/react-redux-thunk-typescript-redux-toolkit-starter-kit/tree/monorep. BUT! - if I explicitly add redux as a dependency to the workspace root, the problem is resolved (and that works in both the test project and my other project using workspaces). So at least there's a workaround for the time being (although obviously it's not ideal to have to add that dependency to the workspace root).

See https://yarnpkg.com/features/pnp#fallback-mode

But again, this only happens using Yarn 2 PnP - all other scenarios I've tried work correctly

It works with node_modules because of hoisting, Yarn PnP on the other hand doesn't allow access to undeclared dependencies.
https://yarnpkg.com/advanced/rulebook#packages-should-only-ever-require-what-they-formally-list-in-their-dependencies

@joekrill
Copy link
Author

Thanks so much @arcanis and @merceyz -- you're absolutely right. Oddly, redux-thunk does have redux listed as a peer dependency since sometime in 2019, but the package on NPM hasn't been updated for 3 years! (There's an issue discussing an update, though).

I had tried fixing any dependency problems using packageExtensions, but this was the only thing yarn flagged related to that.

➤ YN0002: │ typescript-react-redux-redux-saga-starter-kit_version_2@workspace:. doesn't provide redux (p60dc0), requested by react-redux

I wonder why it didn't flag the redux-thunk dependency problem in the same way? I'll have to remember the PNP_DEBUG_LEVEL=1 trick for these sorts of things in the future.

In any case, I really appreciate the help and additional information - I've spent quite a bit of time trying to track down the problem or find a workaround!

@merceyz
Copy link
Member

merceyz commented Jul 21, 2021

I wonder why it didn't flag the redux-thunk dependency problem in the same way?

That check is for peerDependencies that haven't been provided correctly when requested, in this case redux-thunk doesn't request anything so nothing to flag. It would have had to scan the source code to find that redux was undeclared which is a bit out of scope for that check.

@merceyz merceyz closed this as completed Jul 21, 2021
mmcfarland added a commit to microsoft/PlanetaryComputerDataCatalog that referenced this issue Aug 20, 2021
mmcfarland added a commit to microsoft/PlanetaryComputerDataCatalog that referenced this issue Aug 23, 2021
@SeinopSys
Copy link

SeinopSys commented Nov 10, 2022

For any poor souls finding this thread from Google and not being able to understand why they are still getting errors after the issue should seemingly be resolved, in my case the issue was with the definition of the middleware property of of the configureStore's options object. I used:

configureStore({
  reducer: rootReducer,
  middleware: (getDefaultMiddleware) => [...getDefaultMiddleware({ thunk: { extraArgument } })]
});

which destroys the proper return type provided by getDefaultMiddleware. The correct way to provide the parameter is outlined in the Correct typings for the Dispatch type section and it provides alternate options to add more middleware in a type-safe manner.

If all you need is the defaults, then simply remove the spread syntax:

configureStore({
  reducer: rootReducer,
  middleware: (getDefaultMiddleware) => getDefaultMiddleware({ thunk: { extraArgument } })
});

This cleared up the type issues for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants