redux-toolkit icon indicating copy to clipboard operation
redux-toolkit copied to clipboard

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

Open shermify opened this issue 5 months ago • 19 comments

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!

shermify avatar Jan 12 '24 03:01 shermify

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.

markerikson avatar Jan 12 '24 03:01 markerikson

@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?

aryaemami59 avatar Mar 16 '24 18:03 aryaemami59

@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.

markerikson avatar Mar 16 '24 18:03 markerikson

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 🤔

joshuaellis avatar Mar 18 '24 12:03 joshuaellis

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 🤔

I haven't tested it thoroughly, but I believe you can still export the object and use it without destructuring. This can obviously impact the scalability of your code architecture, so it won't work for everyone. I'm not even sure if that's "legally" supposed to work in typescript, because it seems odd, but that's outside my expertise. We are opting to just wait until it's fixed or there's an official conclusion.

I also just noticed the dependencies in my example were somehow totally broken. :grimacing: Apologies to anyone who tried to test with it. It's fixed now.

shermify avatar Mar 18 '24 14:03 shermify

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?

crutchcorn avatar Apr 01 '24 18:04 crutchcorn

@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?

phryneas avatar Apr 01 '24 19:04 phryneas

@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.

crutchcorn avatar Apr 01 '24 19:04 crutchcorn

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

phryneas avatar Apr 01 '24 19:04 phryneas