valtio: `readonly` considered as a breaking change?

Folks, I’ve got a better solution! With #359, we can do Module Augmentation.

https://codesandbox.io/s/react-typescript-forked-qphpi?file=/src/App.tsx:46-125

declare module "valtio" {
  function useSnapshot<T extends object>(p: T): T;
}

You don’t need such a complicated type util.

https://tsplay.dev/mZry9m

I hope eventually TypeScript supports it: https://github.com/microsoft/TypeScript/issues/13923

---

Overall, I think people wouldn’t necessarily want to revert readonly change. It’s technically type breaking, so, to mitigate such cases, let’s add a note in readme. Will open a PR.

Hi, my suggestion in your case is this (or that):

const snap = useSnapshot(state) as typeof state

Does it work? Maybe we should add this in readme.

What about using a wrapper hook for gradual migration?

import { useSnapshot as useSnapshotOrig } from 'valtio'

type DeepWritable<T> = {
  -readonly [P in keyof T]: DeepWritable<T[P]>
}

const useSnapshot = <T extends object>(proxyObject: T) => {
  const snap = useSnapshotOrig(proxyObject)
  return snap as DeepWritable<T>
}

See: https://tsplay.dev/NrGJlm

fyi for others who found this thread, you can’t add this module augmentation in decs.d.ts — it will override the entire module def — can add it in any other regular ts file, tho

I prefer to make it the other way around: useSnapshotLooselyTyped.

I think there will be a lot of use cases where user’s don’t need to mutate these values, yet they have to be passed on to an api or something else that doesn’t expect that readonly addition.

I don’t disagree with this. It’s something I really overlooked when reviewing the PR. But, there are opposite opinions from users. So, at least from the library perspective, it should use correct typing.

To use the same deep-readonly method as the library itself (without re-implementing it as suggested above), I just did this and it seems to work as expected:

type ReadonlyCat = ReturnType<typeof useSnapshot<Cat>>;

Can you share some small code snippet that breaks with readonly. Would love to learn the real cases.

Here’s one such case: Typescript Playground Link

To me, it looks like exactly the case we want to warn.

In my case method consuming the value was defined in another module and it uses auto-generated type annotations of server responses from yet another module, which don’t have readonly keywords anywhere in them.

Yeah, that’s problem, but it’s only the user who know it. So, as should be the right solution.

But in order to do this I had to rely on the “internal” export

In the next version, we don’t unwrap promises, so the type should be simpler. We may revisit about considering exporting the snapshot type.

Personally I would have prefer that to be default behaviour. It defeats the purpose, but if this is something that everybody has to dance around, maybe it is worth making the default

fwiw, the change is requested from a user, and it’s correct typing and allows to catch errors at compile time. At this point, there’s no reason to go back.

Let’s see how this is troublesome for others.

Good idea! I’ll stay on 1.2.8 and until I have added readonly to my props (or somebody else gives a hint how to fix it 😅)

Thanks for reporting. Hm, I thought it’s a proper fix in types, because it’s frozen in JS. But, I overlooked the breaking types in user code. Let me add a note at least in the release page.

Let’s see how this is troublesome for others. If it causes more trouble than benefits, we can consider reverting it.