Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

axelson
Copy link

@axelson axelson commented May 15, 2022

If the declaration is in a .d.ts file instead then you get the following error:

Module '"valtio"' has no exported member 'proxy'.ts(2305)

Here's a code sandbox that illustrates the issue:
https://codesandbox.io/s/react-typescript-forked-8ijjyy?file=/src/App.tsx

@vercel
Copy link

vercel bot commented May 15, 2022

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

Name Status Preview Updated
valtio ✅ Ready (Inspect) Visit Preview May 15, 2022 at 1:47AM (UTC)

@codesandbox-ci
Copy link

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:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
React Typescript (forked) PR

@dai-shi
Copy link
Member

dai-shi commented May 16, 2022

Hi, thanks for the suggestion.

https://codesandbox.io/s/react-typescript-forked-63t5ic?file=/src/decs.ts
I tried renaming to *.ts, but it still shows the type error.
How did you solve the issue?

@axelson
Copy link
Author

axelson commented May 24, 2022

I didn't do anything special to solve the issue. In my actual project valtio simply moving the declare module "valtio" to a js/types.ts worked fine. I'm not sure why it's not working in codesandbox.

@dai-shi
Copy link
Member

dai-shi commented May 24, 2022

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
See that ☝️ , and it works with *.d.ts.

@magicdawn
Copy link
Contributor

what i see is that, this augmentation should be "augmentation"
when add augmentation code in a .d.ts file, vscode complains that "can not find export proxy from valtio".

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
}

@dai-shi
Copy link
Member

dai-shi commented Jul 2, 2022

https://www.typescriptlang.org/docs/handbook/module-resolution.html
I think some configs in tsconfig.json would solve the issue too even with d.ts.
Can anyone investigate? Maybe this? https://www.typescriptlang.org/tsconfig#paths

@magicdawn
Copy link
Contributor

my local environment

image

image

I guess vscode is confusing the declare statement is

csb

https://codesandbox.io/s/react-typescript-forked-8y04wn?file=/src/decs.d.ts

  • i deleted the import type {xx} from './desc.d.ts'
  • with / without the import 'valtio', no errors occurs

weird.

@macarie
Copy link

macarie commented Jul 28, 2022

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 valtio/react through the exports?

If valtio/react is added to the exports (like valtio/vanilla), then we probably could augment the module definition using

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 useLooseSnapshot/useUnsafeSnapshot or something.

@dai-shi
Copy link
Member

dai-shi commented Jul 28, 2022

It looks like TS has issues augmenting re-exported stuff

So the example in #458 (comment)
isn't working correctly, but overwriting the type?

do you think it might be better to allow direct access to valtio/react through the exports?

Hmm, no, I would like to avoid such a workaround.
My suggestion is not to depend on module augmentation, but do type assertion.

    const snap = useSnapshot(state) as typeof state;

You can define useLooseSnapshot on your end likewise.

const useLooseSnapshot = <T extends object>(state: T) => useSnapshot(state) as T;

@macarie
Copy link

macarie commented Jul 28, 2022

So the example in #458 (comment) isn't working correctly, but overwriting the type?

Unfortunately, on my end at least, it's not working correctly 😵‍💫

Hmm, no, I would like to avoid such a workaround. My suggestion is not to depend on module augmentation, but do type assertion.

    const snap = useSnapshot(state) as typeof state;

You can define useLooseSnapshot on your end likewise.

const useLooseSnapshot = <T extends object>(state: T) => useSnapshot(state) as T;

Thanks for the advice! I think I'll go with the local useLooseSnapshot as it might be easier to handle in a team environment and less error-prone.

@cqh963852
Copy link

Overwriting is work for me.

image

image

image


Personal Opinion.

I don't want use a useLooseSnapshot alias.

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

@dai-shi
Copy link
Member

dai-shi commented Sep 19, 2022

It's great to see various opinions. I think what works works.

Thanks guys. Closing this as what we have now seems valid.
Please feel free to open a new discussion.

@dai-shi dai-shi closed this Sep 19, 2022
@magicdawn
Copy link
Contributor

Update 2023, after reading TypeScript-Handbook new modules part
https://www.typescriptlang.org/docs/handbook/modules/reference.html#ambient-modules

Ambient module declarations are easy to confuse with module augmentations since they use identical syntax. This module declaration syntax becomes a module augmentation when the file is a module, meaning it has a top-level import or export statement (or is affected by --moduleDetection force or auto):

01

as gif shows, any top level export / import make .d.ts become module augmentation.
so if u are using a separate d.ts file + code as readme shows, make sure add a top level export {}

@dai-shi
Copy link
Member

dai-shi commented Dec 10, 2023

Thanks. If you have a suggestion to update README, please feel free to open a PR.

@ice-cap0
Copy link

@magicdawn 's suggestion should be added to the README for sure. Just lost a few hours at this

@magicdawn
Copy link
Contributor

total-typescript-essentials has a detailed explaination about Module Augmentation vs Module Overriding, recommendating this free book !

https://www.totaltypescript.com/books/total-typescript-essentials/modules-scripts-and-declaration-files#module-augmentation-vs-module-overriding

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

Successfully merging this pull request may close these issues.

6 participants