-
-
Notifications
You must be signed in to change notification settings - Fork 273
Add note about typescript module augmentation #458
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 d3c8fb6:
|
Hi, thanks for the suggestion. https://codesandbox.io/s/react-typescript-forked-63t5ic?file=/src/decs.ts |
I didn't do anything special to solve the issue. In my actual project valtio simply moving the |
Hmm, okay, so I did some more trials. Looks like what's important is if you import the file or not. https://codesandbox.io/s/react-typescript-forked-tmmij0?file=/src/decs.d.ts |
what i see is that, this augmentation should be "augmentation" I solved this by add such code to app entry index.tsx // augment
import 'valtio'
declare module 'valtio' {
function useSnapshot<T extends object>(p: T): T
} |
https://www.typescriptlang.org/docs/handbook/module-resolution.html |
my local environmentI guess vscode is confusing the
csbhttps://codesandbox.io/s/react-typescript-forked-8y04wn?file=/src/decs.d.ts
weird. |
It looks like TS has issues augmenting re-exported stuff - see microsoft/TypeScript#12607. Would be great if we could get a response from the team 👀 Anyway, @dai-shi do you think it might be better to allow direct access to If declare module 'valtio/react' {
export function useSnapshot<T extends object>(p: T): T
} The other option (that might be easier for everyone at this point) would be to directly export a |
So the example in #458 (comment)
Hmm, no, I would like to avoid such a workaround. const snap = useSnapshot(state) as typeof state; You can define const useLooseSnapshot = <T extends object>(state: T) => useSnapshot(state) as T; |
Unfortunately, on my end at least, it's not working correctly 😵💫
Thanks for the advice! I think I'll go with the local |
Overwriting is work for me.Personal Opinion.I don't want use a Because the data is the same, but the constraints are different. The team will produces two practice scenarios. Team members may get confused or complain about inconsistent spelling. There are some libraries that recommend users to override the type. https://next-auth.js.org/getting-started/typescript#extend-default-interface-properties |
It's great to see various opinions. I think what works works. Thanks guys. Closing this as what we have now seems valid. |
Update 2023, after reading TypeScript-Handbook new modules part
as gif shows, any top level |
Thanks. If you have a suggestion to update README, please feel free to open a PR. |
@magicdawn 's suggestion should be added to the README for sure. Just lost a few hours at this |
total-typescript-essentials has a detailed explaination about |
If the declaration is in a
.d.ts
file instead then you get the following error:Here's a code sandbox that illustrates the issue:
https://codesandbox.io/s/react-typescript-forked-8ijjyy?file=/src/App.tsx