readonly considered as a breaking change?

[wiesson]

A readonly has been introduced in the recent minor 1.2.9 release

a983b8b#diff-b68a2cbc8606fe37f9c2c72867a87ff50a11c2b38a6d9de45a94281f02bf0a4eR252

Is this considered to be a breaking change? The change makes sense to me and it looks useful, but it in my case, I haven't explicitly marked props as readonly. My build breaks in ts strict mode and in order to fix it, I have to mark all props as readonly (I haven thought much about it before, but to me props are always immutable) where I pass data from useSnapshot.

[dai-shi]

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.

[wiesson]

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 😅)

[rvirani1]

This also broke my build. I also agree that it is a useful change even though it is painful in the short term to change types across my whole code base. I do foresee this being troublesome for a lot of users. Would it make sense to add a small info section about this in the README?

[dai-shi]

Thanks for the feedback. I'm not sure if the README is the right place. I added a note in the release note.
I thought about some migration path, but I don't thin we have a way to warn it and pass the build. So, adding a small info makes sense. Maybe at the top of README?

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

[rvirani1]

I found that any case where the type of a key is an array of something, i.e. string[]. With the new update, I'm forced to change this to readonly string[] and make sure anything using or passing that value when read is typed as readonly string[] instead of string[].

[dai-shi]

Yeah, sorry, I know how it's causing the error. My question out of curiosity is how/where you specify string[] in your app code.

[rvirani1]

Let me know if this helps:

import { proxy } from 'valtio'

interface ProxyType {
  someKey: readonly string[]
}

const testProxy = proxy({ someKey: [] }) as ProxyType

function doAThing (param: string[]): void {
  console.log(param)
}

// The call below throws this:
// Argument of type 'readonly string[]' is not assignable to parameter of type 'string[]'.
// The type 'readonly string[]' is 'readonly' and cannot be assigned to the mutable type 'string[]'.
doAThing(testProxy.someKey)

[dai-shi]

Thanks. ProxyType shouldn't be readonly.
I guess you use useSnapshot (or snapshot?), so it's more like this?

import { proxy, useSnapshot } from 'valtio'

interface ProxyType {
  someKey: string[]
}

const testProxy = proxy<ProxyType>({ someKey: [] })

function doAThing (param: string[]): void {
  console.log(param)
}

  // in component
  const snap = useSnapshot(testProxy)
  doAThing(snap.someKey) // snap.someKey is `readonly string[]`

[dai-shi]

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

[mysterybear]

https://codesandbox.io/s/github/mysterybear/valtio-play

@dai-shi should this be throwing a type error? I've wrapped the HouseT prop type with the Readonly utility type and still getting TS2322

[mysterybear]

Ah, someone on TS Discord helped me out

type HouseViewProps = {
  house: DeepResolveType<HouseT>
}

Maybe we could use some docs on this DeepResolveType utility type?

[dai-shi]

Thanks for reporting. I notice that this is a larger issue than I expected. It's not just about readonly change. It reveals the potential design issue.

So, we didn't think DeepResolveType as public API (not directly exported from index.ts). Somebody already reported about the difficulty of typing caused by DeepResolveType (not related with readonly.) #292
The solution was:

  const { houses } = useSnapshot(store) as HouseT

For most users, DeepResolveType didn't make much sense because it was only useful when we use Suspense features. readonly changed this...

I would like to fix this problem (both of them) somehow. It may require a help from TS experts. There's one thing I want to solve: https://twitter.com/dai_shi/status/1480523055105724419

[mysterybear]

@dai-shi someone on TS Discord says

I think the pattern he's looking for is impossible, trying to contain an explicit reference to a type without exporting it.

https://discord.com/channels/508357248330760243/885582487999365200/933899136850341979

[dai-shi]

That's good to know. Then, I will give up using const enum. Let me create a new PR, which technically solves #292, but it doesn't help this issue.

[privatenumber]

I don't mind the readonly change (although I agree this should've been a major release).
But I wonder if you should also export the Snapshot type utility to recursively convert types to readonly.
Otherwise, I believe I have to create or import a separate DeepReadOnly utility type.

[dai-shi]

Thanks for the feedback.

(although I agree this should've been a major release).

(I feel like I shouldn't have merged that PR, if I knew this.)

I have to create or import a separate DeepReadOnly utility type.

but, that's what I'm suggesting if you really need it. I really want to hide Snapshot type as an implementation detail.

[xzilja]

I stumbled upon a minor issue with readonly:

1. I receive some data from api that I want to persist in valtio
2. I then retrieve it via useSnapshot and need to pass to another api call
3. I now get type error due to added readonly

Is there any way to not add them and keep original types?

[dai-shi]

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.

[xzilja]

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.

Does the trick, but feels a bit like a "hack around". Would it be viable to instead introduce something like useReadonlySnapshot for those who want these strict typings and perhaps enforce it over useSnapshot via valtios eslint plugin with added rule?

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.

[dai-shi]

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.

[xzilja]

That makes sense, perhaps useProxy can be re-used for this? To make naming nicer. This might also help with upgrading from older versions of valtio without that babel macro?

[dai-shi]

useProxy macro is totally a different one after v1, so we shouldn't mix it.

currently I'm leaning to a recipe in readme rather than an exported function in utils, because there could be variations for various use cases.

Let's see what others think.

[Elia-Darwish]

We have a project where we are using a generated API types where readonly is not supported, which made the process even longer. But we finally have it done!!

I just wanted to share a Typescript snippet that we used to ease the process...

declare type AnyFunction = (...args: unknown[]) => unknown

export type DeepReadonly<T> = T extends (infer R)[]
  ? DeepReadonlyArray<R>
  : T extends AnyFunction
  ? T
  : T extends object
  ? DeepReadonlyObject<T>
  : T

export type DeepReadonlyArray<T> = ReadonlyArray<DeepReadonly<T>>

export type DeepReadonlyObject<T> = {
  readonly [P in keyof T]: DeepReadonly<T[P]>
}

@dai-shi would it maybe be helpful to export a similar utility to ease the process of migration?

[dai-shi]

You don't need such a complicated type util.

https://tsplay.dev/mZry9m

I hope eventually TypeScript supports it: microsoft/TypeScript#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.

[dai-shi]

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;
}

[tarngerine]

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

[dai-shi]

We have a PR #458 open to document it.

[dai-shi]

#458 is closed but the discussion there might be valuable.

[some-dudes-work-account]

Stumbled on this as well earlier this week. I know the discussion has long been over but let me contribute anyway

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

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.

I like the idea of having readonly properties on objects returned from useSnapshot and have awaited promise values unwrapped. And the only thing that TS didn't like seems to be when we're attempting to pass ReadonlyArray's in place of Array's (same for readonly tuples), so rather than re-defining it as

function useSnapshot<T extends object>(p: T): T;

as was suggested above I went for a different approach and just undid readonly flags on arrays. But in order to do this I had to rely on the "internal" export

// somewhere in my lib-env.d.ts

import 'valtio'; // probably unnecessary because of the next line
import { INTERNAL_Snapshot } from 'valtio';

type AnyFunction = (...args: any[]) => any;
type AnyArray = readonly unknown[];
type MutableArrays<T> = T extends AnyFunction ? T :
  T extends AnyArray
    ? { -readonly [K in keyof T]: MutableArrays<T[K]> }
    : { [K in keyof T]: MutableArrays<T[K]> }; // prettier-ignore

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

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

[dai-shi]

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.

[vincerubinetti]

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>>;

[dai-shi]

I didn't know it was possible.

https://tsplay.dev/m3q5qw

Seems like it's already supported in TS4.7.
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-7.html#instantiation-expressions

Thanks for sharing!

[dai-shi]

And this: https://tsplay.dev/WPJZZN

[Noitidart]

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

Thank you, this was causing me problems as well. I was putting it into global.d.ts which also override the whole module. I put it in the index.ts file of app entry and it doesn't override, but augments.