`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.