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

(RTK Alpha 2): Cannot resolve types when moduleResolution is set to node16 #3202

Closed
eric-crowell opened this issue Feb 24, 2023 · 32 comments
Closed
Milestone

Comments

@eric-crowell
Copy link
Contributor

eric-crowell commented Feb 24, 2023

A follow up on #2485. When importing anything from @reduxjs/toolkit@2.0.0-alpha.2, typescript cannot resolve types when tsconfig.json's moduleResolution is set to node16 or nodenext.

I found that adding the extension .js to the declaration imports resolved the issue. For example....

In my file: ./node_modules/@reduxjs/toolkit/dist/index.d.ts
export { createSlice, } from './createSlice'; -> export { createSlice, } from './createSlice.js';

Two suggestions for a fix: First, maybe the easiest, is add a post build script to add the extensions to the output. Or second, a search and replace to add the extensions to the all the relevant source files. Want to get some thoughts on a direction before attempting a PR. Maybe there's a better way.

I tried looking to see if typescript can just add them while compiling... but didn't find anything that could.

@markerikson
Copy link
Collaborator

Oh boy. Honestly I've never even tried to mess with TS's resolution options. I've seen a couple of the discussion threads, but I really don't know what's actually needed for any of that.

I'm really not a fan of the ".js" extension in import paths, so I'd really prefer to avoid that if possible.

@markerikson markerikson added this to the 2.0 milestone Feb 24, 2023
@eric-crowell
Copy link
Contributor Author

I feel you. It's been a painful transition going into ESM. I've been searching long and wide for some backwards compatibility. I have some packages that rely on the 'exports' attribute in their package.json, which apparently only works when that Node16 module resolution is set. Otherwise, it's like exports doesn't exist.

As far as this reads, the extensions are required: https://nodejs.org/api/esm.html#mandatory-file-extensions

There's an eslint plugin that has helped me transition to these ESM requirements: eslint-plugin-require-extensions. Allows eslint --fix to add most of those extensions. Scary when seeing a ton of file modifications though.

Ecosystem is in a limbo, haha.

@markerikson
Copy link
Collaborator

If I'm reading https://twitter.com/AndaristRake/status/1629168415175835649 correctly, the suggested fix here is actually to drop type: "module" from the RTK package.json.

@incepter
Copy link

Hello guys; I hope you are doing well.

I did reproduce this issue in this repo (if you want it as a starting point): https://github.com/incepter/alpha-toolkit-test
(first google result of: redux toolkit typescript codesandbox)

Actually, the solution turns to be quit simple: Just remove "type": "module" from package.json. Is there any need for it? afaik there are no .mjs files in the repo ? There is main and module and types entries, I think (my knowledge is very limited though) that this is enough.

@eric-crowell
Copy link
Contributor Author

eric-crowell commented Feb 24, 2023

Thank you! Unfortunately, removing the "type": "module" doesn't appear to work for my use case. Maybe the issue is using tsc to output declarations files for a library-type package. Building with vite certainly did not give me any issues though! So that's a step.

@incepter I ran into some similar problems when trying to output some declaration files with tsc in your example:

# ...
src/Toggle.tsx:7:41 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("@reduxjs/toolkit")' call instead.
  To convert this file to an ECMAScript module, add the field `"type": "module"` to './alpha-toolkit-test/package.json'.

7 import { ActionReducerMapBuilder } from "@reduxjs/toolkit";
# ...

@rschristian
Copy link
Contributor

If I'm reading https://twitter.com/AndaristRake/status/1629168415175835649 correctly, the suggested fix here is actually to drop type: "module" from the RTK package.json.

Sorry, but unfortunately that's incorrect. You'd need to switch the ESM entries in "exports" to use .mjs, which will have the exact same issue.

This is the direction the TS team have chosen to go (sadly), you'll need your .d.ts files to use .js in import specifiers if you want to support the node16 module resolution w/ ESM.

@markerikson
Copy link
Collaborator

Well, frankly, now I'm completely lost.

Seriously.

I don't know what I am supposed to do :(

@rschristian
Copy link
Contributor

rschristian commented Feb 25, 2023

So this particular issue is quite simple, you just need to ensure the .d.ts files refer to each other with .js. TS will do this for you, so long as you use .js in your import specifiers when referring to local .ts files. Wonky, yes, but it's the way they've gone.

The desired output of ./dist/index.d.ts looks like this:

...
-export { createDraftSafeSelector } from './createDraftSafeSelector';
+export { createDraftSafeSelector } from './createDraftSafeSelector.js';
...

I haven't taken a peek at your build tooling (or attempted to run it), but in theory, it should be as simple as altering this:

export { createDraftSafeSelector } from './createDraftSafeSelector'

to be this:

export { createDraftSafeSelector } from './createDraftSafeSelector.js'

Running tsc itself over that will create the desired .d.ts file as output, as it's one of TS's fundamental rules to not rewrite valid JS (which an import like that is, under some interpretations).

@markerikson
Copy link
Collaborator

Seems like microsoft/TypeScript#49160 is a related "everything is broken now" issue.

@markerikson
Copy link
Collaborator

export { createDraftSafeSelector } from './createDraftSafeSelector.js'

I realize this is going to sound petty and has no sound basis.

But that looks stupid, and I really don't want to do that :(

@markerikson
Copy link
Collaborator

https://arethetypeswrong.github.io/ looks useful

@rschristian
Copy link
Contributor

I do agree, but sadly, that's their direction and they're seemingly sticking to it.

You could always merge type declarations, so there's a mega .d.ts and no references that could be invalid, but that's a bit of work on top of normal output behavior.

@eric-crowell
Copy link
Contributor Author

So I started patching @reduxjs/toolkit@2.0.0-alpha.2 myself with various changes; such as removing "type": "module" and adding all this .js extensions to the .d.ts files. It fixes issues directly in @reduxjs/toolkit, but there are still typescript issue if I don't also patch immer, redux, redux-thunk, and reselect.... and probably deeper in the dependencies of those packages too.

I'm about convinced this effort to support node16 module resolution is unreasonably difficult without TypeScript supporting some kind of compatibility without those .js extensions... at least until the majority have shifted over.

I don't know. I am definitely looking forward to updates from that discussion mentioned on Twitter.

@phryneas
Copy link
Member

phryneas commented Feb 26, 2023 via email

@markerikson
Copy link
Collaborator

I really, really don't want to do that :(

@eric-crowell
Copy link
Contributor Author

eric-crowell commented Feb 26, 2023

I created the needed patches to the Redux alpha package family to resolve this types issue. Might be helpful to those that use PNPM or Yarn and still want to experiment with the alpha: https://github.com/eric-crowell/redux-toolkit-alpha.2-patches/tree/main/patches
I sure needed to in order to utilize the exports feature.

I had to export some additional types too or else encounter the error The inferred type of 'X' cannot be named without a reference to 'Y'. This is likely not portable. A type annotation is necessary.
https://github.com/eric-crowell/redux-toolkit-alpha.2-patches/blob/2ce7d86c3a41313384a1cf0df31d68fbc08ee7e0/patches/%40reduxjs__toolkit%402.0.0-alpha.2.patch#L773-L775

I wouldn't mind working this effort to add the extensions for alpha 3 (or as you like). That way, you wouldn't have to focus on it too much. Maybe IDEs can optionally hide them for aesthetic purposes soon.

If it's any consolation, I am also sad about the lack of alternatives.

@markerikson
Copy link
Collaborator

If anything, I'd rather investigate a new build step to rewrite the generated .d.ts files to have the right import extensions.

@eric-crowell
Copy link
Contributor Author

eric-crowell commented Feb 26, 2023

Alright. I suppose there's a need to wait on some of those test apps against the build output then.

Otherwise, could be worth looking into running several instances of vitest configured for various environments and tsconfigs to verify compatibilities. (Maybe)

Want to help out where I can. Really do appreciate everything!

@markerikson
Copy link
Collaborator

markerikson commented Feb 26, 2023

@eric-crowell : that's roughly what I'm attempting over in #3213 .

Yesterday I managed to get a PR initially passing that takes the packed artifact we were already generating (eh... technically I skipped the "generated" part because I needed to iterate faster on the GH Actions job and Playwright step, but that part works already), installs it into a CRA 5 app, builds it, and runs a Playwright test that verifies basic functionality (counter increments, two RTKQ APIs run (with and without React)).

I had done some work a few weeks ago to make local projects for CRA, Next, Vite, and Expo. So now that I can do that for one example, I can start copy-pasting the example projects in and matrix them. That will help verify that any given PR builds and runs correctly.

Once I get that working for 1.9.x on master, we can merge that into the v2.0-integration branch and start using that to check the alpha changes.

Actually, if you or someone else really wants to help... you could start filing PRs targeted at that PR to add equivalent setups for a variety of apps built with different tools.

Off the top of my head, I'd like to target:

  • CRA 4 (w/ Webpack 4)
  • CRA 5 (w/ Webpack 5)
  • Next (also Webpack 5)
  • Vite (ESBuild + Rollup)
  • Expo (which uses Webpack 4 for its "web" target, and I assume Metro for the RN targets)
  • Standard RN
  • Parcel
  • Astro? (which I think uses Vite, but per Alpha 2.0 - Cannot read properties of undefined (reading 'createApi') - Astro #3176 someone reported the alpha isn't loading right )
  • Node, both in CJS and ESM modes
  • Somehow a couple variations on TS's resolution options
  • Jest and Vitest too?

The other immediately open question would be how to do the equivalent of a Playwright test for an RN app. I'm sure there's something similar, I just haven't looked (and I don't use RN myself).

The app code in the example should be portable across all the examples (store setup, RTKQ API definitions, most of the components). The components would need to be redone for the RN examples (and I have a couple of those bashed together locally I think).

So, porting the example app setups would be a huge help!

@eric-crowell
Copy link
Contributor Author

Awesome- so if I understand correctly, one would...

  • branch off of feature/rtk-apps-ci
  • initialize one of targets on that list (e.g. Next w/ WebPack 5 with create-next-app) under the examples/publish-ci folder
  • add that generated folder name to the action matrix (e.g. nextjs)
  • Setup RTK/RTKQ in it like you did for CRA5 (portable across all the examples)
  • Write a playwrite test (or equivalent) to verify end-to-end functionality using MSW to handle the requests
  • Create a PR back into feature/rtk-apps-ci

I haven't started a RN app in quite some time. It looks like they mentioned Detox for end-to-end testing on their site: https://reactnative.dev/docs/testing-overview#end-to-end-tests

@markerikson
Copy link
Collaborator

@eric-crowell : pretty much, yeah.

I've actually got Next and Vite examples running locally from when I was playing with this a few weeks ago, so let me go ahead and add those to my PR. If you can tackle any of the others, that'd be great!

For RN, we don't have official Redux templates, but there's a couple unofficial templates at https://github.com/rahsheen/react-native-template-redux-typescript and https://github.com/rahsheen/expo-template-redux-typescript .

@eric-crowell
Copy link
Contributor Author

@markerikson Sounds good! Following your examples, I'll start by giving a swing at Expo React Native-

@markerikson
Copy link
Collaborator

@eric-crowell : I was thinking about it this morning, and I'm now actually wondering if it makes sense to keep these "CI Examples" in a separate repo.

We're going to be making similar build tooling changes for the various other Redux repos ( redux, redux-thunk, reselect, and maybe react-redux). The example apps actually test all those together. It might make sense to actually have the examples in their own repo, so that each lib repo's CI can clone the examples and run them.

Tell you what. Go ahead and file PRs that target this branch for now, and we can extract them to their own repo later.

@eric-crowell
Copy link
Contributor Author

eric-crowell commented Feb 27, 2023

@markerikson That's not a bad idea! If you're wanting to do create the new repo soon, I still need some time for this PR. I can create a new branch and copy the work over to it. No problem.

End-to-end tests for React Native projects certainly aren't as straight forward. More extensive with needing emulators and specific SDKs. Here's the PR I was testing with on my own forked repo: eric-crowell#1

The build is working with Metro from what I see reading the cli output. However, getting a virtual device for Detox to use (on GitHub Actions) is where I left off. Maybe someone out there has a good simple workflow setup with Detox I can follow.

@eric-crowell
Copy link
Contributor Author

@markerikson Here's my completed setup of a React Native (w/ Metro bundler) end-to-end test project, which very closely mirrors your cra5 example: https://github.com/eric-crowell/react-native-redux-e2e

I'll update that project along with changes made in #3213 . Don't want to step on any toes as you make your refinements.

@markerikson
Copy link
Collaborator

@eric-crowell : awesome! haven't had time to touch this since I put up that PR, but I'm doing some traveling next week and may work on it while I'm flying.

@eric-crowell
Copy link
Contributor Author

Safe travels!

@markerikson
Copy link
Collaborator

@eric-crowell : I may have changed my mind one more time :)

All of the other repos (Redux core, Reselect, React-Redux) would need to clone a repo to get their hands on these example projects in their CI jobs, in order to do the install + build + test steps...

but if they have to clone a repo anyway, it could be this repo instead of a separate repo.

Actually going to spend today's flying time focusing on some docs feedback I just got ( reduxjs/redux#4495 ), but very much still intending to get more example apps integrated into that branch ASAP.

@markerikson
Copy link
Collaborator

I think we can call this issue fixed as of the latest 2.0 alphas.

@ryerrappa
Copy link

Hi @markerikson
I've been getting a similar error with RTK 2.0.1. Error is:

Cannot find module @reduxjs/toolkit/dist/query/react/buildHooks

Occurs when trying to import the MutationTrigger type. Using vite with the following tsconfig

{
  "compilerOptions": {
    "composite": true,
    "skipLibCheck": true,
    "module": "ESNext",
    "moduleResolution": "bundler",
    "allowSyntheticDefaultImports": true
  },
  "include": ["vite.config.ts"]
}

@markerikson
Copy link
Collaborator

@ryerrappa please open up a new issue, and include a repo that shows this happening. We specifically removed all the /dist references in the types, so that shouldn't be happening any more.

@ryerrappa
Copy link

Thank you for the prompt response. I've created an issue here: #3970

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