pmndrs / valtio

💊 Valtio makes proxy-state simple for React and Vanilla
http://valtio.pmnd.rs
MIT License
8.7k stars 244 forks source link

fix(utils): improve devtools typing to avoid depending on @redux-devtools/extension #777

Closed fukumasuya closed 11 months ago

fukumasuya commented 11 months ago

Related Issues or Discussions

Fixes #776

Summary

I found the same issue in zustand(https://github.com/pmndrs/zustand/issues/1205#issuecomment-1221270524), so I added the same workaround(copied the type defs from @redux-devtools/extension) I'm not sure if this way is a desired way, but I did it anyway..

Check List

vercel[bot] commented 11 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
valtio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2023 10:29am
codesandbox-ci[bot] commented 11 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3dc8f03b4b5111b405bfa6b6a02debcc9be49a31:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
fukumasuya commented 11 months ago

It might be a better solution to move @redux-devtools/extension from devDependencies to dependencies and import Config type from @redux-devtools/extension .. But it might seem a little odd that it's in dependencies and redux is a peerDependencies of @redux-devtools/extension and devtools imports redux, so valtio users need to install redux manually...

fukumasuya commented 11 months ago

So, it's basically doing the same as https://github.com/pmndrs/zustand/pull/1207 ?

Yes. that's right.

I also confirmed https://github.com/pmndrs/valtio/pull/777/commits/208d3c9b1d6ee655d0ac332625204fd0e8477c6d fixes #776. thanks for updating my fix!

But it looks #713 feature does not work with TypeScript.. (tsc fails when we add some parameters that is coming from @redux-devtools/extensions.. is that ok for us?

const state = proxy({ count: 0, text: 'hello' });
devtools(state, { name: 'state name', enabled: true, latency: 1000 }); // 'latency' is coming from `@redux-devtools/extensions`
  Overload 1 of 2, '(proxyObject: { count: number; text: string; }, options?: { enabled?: boolean | undefined; name?: string | undefined; } | undefined): (() => void) | undefined', gave the following error.
    Argument of type '{ name: string; enabled: true; latency: number; }' is not assignable to parameter of type '{ enabled?: boolean | undefined; name?: string | undefined; }'.
      Object literal may only specify known properties, and 'latency' does not exist in type '{ enabled?: boolean | undefined; name?: string | undefined; }'.
  Overload 2 of 2, '(proxyObject: { count: number; text: string; }, name?: string | undefined): (() => void) | undefined', gave the following error.
    Argument of type '{ name: string; enabled: boolean; latency: number; }' is not assignable to parameter of type 'string'.

15 devtools(state, { name: 'state name', enabled: true, latency: 1000 });
fukumasuya commented 11 months ago

It seems if we install @redux-devtools/extension and import some function in this or load the type def file, then __REDUX_DEVTOOLS_EXTENSION__ is declared and following code works without type errors.., In that case, we need to install redux additionally because redux is in peerDependencies of @redux-devtools/extension..

const state = proxy({ count: 0, text: 'hello' });
devtools(state, { name: 'state name', enabled: true, latency: 1000 }); // 'latency' is coming from `@redux-devtools/extensions`

tsconfig.json

{
  "include": [
    "./node_modules/@redux-devtools/extension/lib/types/index.d.ts",
  ]
}
dai-shi commented 11 months ago

Hmm, isn't it just enough to install @redux-devtools/extension? In that case, we should update the docs to explicitly import it (both in valtio and zustand.)

import type {} from '@redux-devtools/extension'
import { devtools } from 'valtio/utlis'
fukumasuya commented 11 months ago

Hmm, isn't it just enough to install @redux-devtools/extension?

It seems If we set skipLibCheck to true, we don't need to install redux. Anyway, https://github.com/pmndrs/valtio/pull/777/commits/208d3c9b1d6ee655d0ac332625204fd0e8477c6d fixes #776 and is fine for me 👍 thanks!