reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.58k stars 1.14k forks source link

Typescript errors using Node ESM module resolution and "declaration: true" #4066

Closed shermify closed 3 days ago

shermify commented 6 months ago

I noticed myself and a few other people getting an error The inferred type of X cannot be named without a reference to '../../node_modules/@reduxjs/toolkit/dist/query/react/buildHooks'. This is likely not portable. A type annotation is necessary.

This happens when you have declaration: true or you're using composite projects as can be seen in my very simple reproduction here. https://github.com/shermify/redux-toolkit-ts-repro

The error in slice.ts Screenshot from 2024-01-11 20-05-08

I believe the problem is that many of the types are not explicitly exported from redux-toolkit. In this case, it's looking for UseQuery, but there are many others that give me problems. I think this is an issue because ESM modules do not allow you to reach into packages or files that are not declared explicitly in the package.json. Indeed, changing module resolution to "Node" solves it, but "NodeNext", "Node16", and "bundler" have problems. "Node" resolution is not suggested for new projects.

Exporting the types explicitly from redux-toolkit fixes it, also adding the types to the export section of the package.json of redux-toolkit fixes it. However, there are a bunch of types that cause this error, so I'm not sure what the best solution would be. I don't think there is a workaround because the user cannot actually import the necessary types even if they wanted to manually due to ECMAScript constraints.

Screenshot from 2024-01-11 20-13-29

The use case for "declaration: true" would be, for example, creating a library with redux-toolkit actions or having a monorepo with shared packages. Let me know if any of this sounds incorrect!

markerikson commented 6 months ago

That sounds plausible. @eric-crowell was doing some contributions prior to RTK 2.0 related to checking type exports for this sort of thing ( https://github.com/reduxjs/redux-toolkit/pulls?q=is%3Apr+author%3Aeric-crowell ), but it's possible more changes are needed here.

aryaemami59 commented 4 months ago

@markerikson Do you think it would be worth it to try and see if it would be plausible to bundle the type definitions into one file like we're doing with the other repos?

markerikson commented 4 months ago

@aryaemami59 : maybe. As mentioned, I ran into issues attempting to bundle them while working on RTK 2.0. tsup's bundler didn't correctly separate them out by entry point.

joshuaellis commented 4 months ago

Hey folks, we're wanting to upgrade Strapi to use v2 of rtk-query but are facing this issue, is there any known workaround or potential route to investigate 🤔

crutchcorn commented 4 months ago

I've been running into a few of these errors:

src/store/client-slice.ts(95,14): error TS2742: The inferred type of 'setClient' cannot be named without a reference to '../../node_modules/@reduxjs/toolkit/dist/createAsyncThunk'. This is likely not portable. A type annotation is necessary.

When working on TanStack Form packaging with @lachlancollins, we found that we could not reliably trust tsup to work as-expected.

Moreover, it looks like you're only generating one set of .d.ts files, where most tooling expects dedicated .d.ts esm syntax and .d.cts commonjs syntax exports, annoyingly.

Together, these exports might look something like this:

https://github.com/TanStack/form/blob/main/packages/form-core/package.json#L17-L27

And to support this build pipeline, we (TanStack) have created a new project called Config that builds on top of Vite instead of TSUp and has proven much more reliable for our needs.

This is an example of what a configuration looks like with Config: https://github.com/TanStack/form/blob/main/packages/form-core/vite.config.ts

@markerikson, were I to open an experimental PR that migrates toolkit to the Config tooling that fixes these problems, would you be willing to consider it?

phryneas commented 4 months ago

@crutchcorn sorry for kinda hijacking this, but I'd be very interested to see if this is expressive enough to handle something like this setup: https://github.com/apollographql/apollo-client-nextjs/blob/pr/rsc-preload/packages/client-react-streaming/tsup.config.ts

What do you think?

crutchcorn commented 4 months ago

@phryneas I don't see why it wouldn't!

Unlike TSUp, which abstracts a lot of the internals of the build tooling, we're just a slot-in configuration with some default plugins for Vite to make the basics easier to tackle while still making intense customizations easier.

You still have full control over adding any other plugins (custom or otherwise) and configuration overwrites.

phryneas commented 4 months ago

@crutchcorn Thanks, I'll take a closer look at it then when I get around to it then! :)

chenhebing commented 2 months ago

I would like to know if there is any new progress on this issue? Thanks

markerikson commented 2 months ago

@chenhebing : generally if the issue is open and there's no further comments or linked PRs, there's been no progress :)

aryaemami59 commented 2 months ago

I am still working on resolving this issue in a way that doesn't involve exporting internal types, so far I've had little success with it, if I make any progress I will be sure to keep everyone updated.

CarlRibbegaardh commented 2 months ago

Here's some help with the troubleshooting part. I recreated and fixed the issue locally using this setup: https://github.com/reduxjs/redux-templates/blob/master/packages/cra-template-redux-typescript/template/

1. Setup project

Could not use npx create-react-app my-app --template redux-typescript because it gives not the same template content. Manual downloaded this repo and used the "cra-template-redux-typescript" https://github.com/reduxjs/redux-templates/blob/master/packages/cra-template-redux-typescript/template/

2. Change tsconfig.json:

declaration: true

VS Code errors on multiple places.

3. Exported variable 'rootReducer' has or is using name 'QuotesApiResponse' from external module "c:/Projects/RTK/my-app/src/features/quotes/quotesApiSlice" but cannot be named.ts(4023)

Change quotesApiSlice.ts from interface QuotesApiResponse { to export interface QuotesApiResponse {

4. The inferred type of 'useGetQuotesQuery' cannot be named without a reference to '../../../node_modules/@reduxjs/toolkit/dist/query/react/buildHooks'. This is likely not portable. A type annotation is necessary.

Change file \my-app\node_modules.pnpm\@reduxjs+toolkit@2.2.4react-redux@9.1.2@types+react@18.3.0_react@18.3.1_redux@5.0.1__react@18.3.1\node_modules\@reduxjs\toolkit\dist\query\react\index.d.ts

Add at the end of the file: *export from './buildHooks';**

5. The inferred type of 'rootReducer' cannot be named without a reference to '../../node_modules/@reduxjs/toolkit/dist/combineSlices'. This is likely not portable. A type annotation is necessary.ts(2742)

Change file \my-app\node_modules.pnpm\@reduxjs+toolkit@2.2.4react-redux@9.1.2@types+react@18.3.0_react@18.3.1_redux@5.0.1__react@18.3.1\node_modules\@reduxjs\toolkit\dist\index.d.ts

Add at the end of the file: *export from "./combineSlices";**

No more errors reported from VS Code. 😀

aryaemami59 commented 1 month ago

I should have the solution for this ready soon. In the meantime if you guys could provide me with as many examples and code snippets as possible that would be great. This is not the easiest thing to test for.

aryaemami59 commented 1 month ago

@CarlRibbegaardh Fun fact about the TS4023 error, if you convert the problematic type from an interface to a type alias, the problem goes away, of course in your case, I'm willing to bet you'll get a different error, but it's just weird to me how that error works, considering interfaces and type aliases are supposed to be somewhat interchangable.

@shermify Thank you I have your original example already, I've managed to fix it, just need more examples and code snippets to test so we can scan for problematic types that are causing this issue.

joshuaellis commented 1 month ago

@aryaemami59 are you able to do an pre-release? I might be able to test it in the Strapi repo with the help of @jhoward1994

mickosav commented 1 month ago

If you folks are using pnpm maybe take a look at this: https://github.com/microsoft/TypeScript/issues/47663#issuecomment-1270716220

It helped me solve the

The inferred type of X cannot be named without a reference to '../../node_modules/@reduxjs/toolkit/dist/query/react/buildHooks'. This is likely not portable. A type annotation is necessary.

issue without disabling declarations.

aryaemami59 commented 1 month ago

@joshuaellis I'm not a collaborator on this repo so I can't do a "pre-release", I tested the new build in the Strapi repo, it seems to work fine, but I'm not as familiar with Strapi so if you wanna test it, this is where I'm at as of now.

stewartmoreland commented 3 weeks ago

any update on this PR?

I'm attempting to configure this in my monorepo and encountering the same issues.

services/client.ts:3605:1242 - error TS2742: The inferred type of 'useLazyListUsersQuery' cannot be named without a reference to '../../../node_modules/@reduxjs/toolkit/dist/query/react/buildHooks'. This is likely not portable. A type annotation is necessary.
CarlRibbegaardh commented 3 weeks ago

@stewartmoreland, I believe @aryaemami59 is still working on a PR. I saw an update from today in another thread.

Meanwhile you can patch (pnpm patch or similar) using the technique I did above. As the patches are always tied to a specific version, an upgrade would invalidate the patch, so it will not cause future issues.

aryaemami59 commented 3 weeks ago

@CarlRibbegaardh @stewartmoreland

@stewartmoreland, I believe @aryaemami59 is still working on a PR. I saw an update from today in another thread.

Yeah I'm still working on it, I should be done soon, in the meantime it would really help if you guys could run this command in your CLI and let me know if it fixes the issue you're having:

npm install https://pkg.csb.dev/reduxjs/redux-toolkit/commit/06a30ee4/@reduxjs/toolkit
CarlRibbegaardh commented 3 weeks ago

@CarlRibbegaardh @stewartmoreland

@stewartmoreland, I believe @aryaemami59 is still working on a PR. I saw an update from today in another thread.

Yeah I'm still working on it, I should be done soon, in the meantime it would really help if you guys could run this command in your CLI and let me know if it fixes the issue you're having:

npm install https://pkg.csb.dev/reduxjs/redux-toolkit/commit/beb818cb/@reduxjs/toolkit

I couldn't verify the content of that url as it's in a binary format, and I don't recognize the source, so I'm not going to run it. (I did search the commit id and found the originating source.)

Text based patches is another thing. :)

markerikson commented 3 weeks ago

That is a tgz tarball with the published package from the PR, generated by the CodeSandbox CI job in that PR.

You can see the URL by opening the details link for that job.

Trust me, that's safe to install. It's the exact same binary archive that would get uploaded to npm if we published that commit live.

knotekbr commented 2 weeks ago

@stewartmoreland, I believe @aryaemami59 is still working on a PR. I saw an update from today in another thread.

Meanwhile you can patch (pnpm patch or similar) using the technique I did above. As the patches are always tied to a specific version, an upgrade would invalidate the patch, so it will not cause future issues.

@CarlRibbegaardh thanks a ton for this workaround! I encountered this issue in a yarn workspaces monorepo today, and at this point it's been driving me up the wall for hours. The patch approach (specifically steps 4 + 5) worked beautifully. As a bonus, I had never used yarn patch before, so that's a new tool in my toolbox.

shermify commented 2 weeks ago

Works for me. Thanks @aryaemami59 for all your hard work on this!

markerikson commented 3 days ago

Fixed in https://github.com/reduxjs/redux-toolkit/releases/tag/v2.2.7 !