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 using Node ESM module resolution and "declaration: true" #4066

Closed
shermify opened this issue Jan 12, 2024 · 25 comments
Closed
Labels
bug Something isn't working rtk-query typescript
Milestone

Comments

@shermify
Copy link

shermify commented Jan 12, 2024

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
Copy link
Collaborator

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
Copy link
Contributor

@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
Copy link
Collaborator

@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
Copy link

joshuaellis commented Mar 18, 2024

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 🤔

@reduxjs reduxjs deleted a comment from pebubblemaps Mar 18, 2024
@markerikson markerikson added bug Something isn't working rtk-query typescript labels Mar 18, 2024
@crutchcorn
Copy link
Member

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
Copy link
Member

phryneas commented Apr 1, 2024

@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
Copy link
Member

@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
Copy link
Member

phryneas commented Apr 1, 2024

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

@chenhebing
Copy link

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

@markerikson
Copy link
Collaborator

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

@aryaemami59
Copy link
Contributor

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
Copy link

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.4_react-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.4_react-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 added a commit to aryaemami59/redux-toolkit that referenced this issue Jun 8, 2024
- This should serve as a partial fix for reduxjs#3962, reduxjs#4448, reduxjs#3983, reduxjs#4066, reduxjs#4108, reduxjs#4401
- Here is the list of the problematic (now exported) types:
  From `@reduxjs/toolkit`:
  - `CombinedSliceReducer`
  - `CaseReducerDefinition`
  - `Id` renamed to `TSHelpersId`
  - `UncheckedIndexedAccess`
  - `ReducerWithInitialState`
  - `CaseReducerDefinition`
  - `Id` renamed to `TSHelpersId`
  - `UncheckedIndexedAccess`
  - `ReducerWithInitialState`
  From `@reduxjs/toolkit/query/react`:
  - `UseLazyQuery`
  - `UseQuery`
  - `QueryHooks`
@aryaemami59
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link

@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
Copy link

mickosav commented Jun 13, 2024

If you folks are using pnpm maybe take a look at this: microsoft/TypeScript#47663 (comment)

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
Copy link
Contributor

@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
Copy link

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
Copy link

@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
Copy link
Contributor

aryaemami59 commented Jul 9, 2024

@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
Copy link

@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
Copy link
Collaborator

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
Copy link

@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
Copy link
Author

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

@markerikson
Copy link
Collaborator

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

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

No branches or pull requests