TypeScript Templates: Confusion around ReadOnly about next.js

[eps1lon]

I don't see how adding ReadOnly fixes any of these issues?

Maybe this helps illustrating it: Playground Link

[jinmayamashita]

Hi, @eps1lon

As you pointed out, Readonly only protects shallow properties, so local variables created through destructuring can still be reassigned.

With this in mind, how about using props instead of { children }?
Although the code might be slightly longer, it doesn't significantly impact readability or performance.

This way, type checking will catch any reassignment attempts, and developers will be reminded to avoid them.

export default function RootLayout(props: Readonly<{ children : React.ReactNode }>) {
 // ...
}

Or, you could just remove Readonly when destructuring function parameters.
I think this approach is fine too, since local bindings can be reassigned anyway.

export default function RootLayout({ children }: { children : React.ReactNode }) {
 // ...
}

[mirasayon]

In the RootLayout function, children is a parameter, not a property of the object. You can reassign function parameters in JavaScript/TypeScript, even if they are explicitly marked as read-only. Live with it.
I think TypeScript will add this feature soon.

[jinmayamashita]

@mirasayon That's a good point.

However, it's crucial to note that when using the ReadOnly type, properties should remain read-only to prevent unintended modifications.

The main issue lies with destructuring. Even if the original props object is read-only, destructuring creates new variables that are no longer read-only. This can undermine code stability. If TypeScript doesn't fully support this aspect, it should be improved to avoid confusion for users of the template.

[eps1lon]

However, it's crucial to note that when using the ReadOnly type, properties should remain read-only to prevent unintended modifications.

ReadOnly is shallow. It's only meant to catch props.children = somethingElse. children = somethingElse is perfectly fine since you're not reassigning props.children but the local binding which can be reassigned by spec.

You'd need another lint rule to prevent that. I don't see how removing ReadOnly fixes any of these issues?

[mirasayon]

However, it's crucial to note that when using the ReadOnly type, properties should remain read-only to prevent unintended modifications.

ReadOnly is shallow. It's only meant to catch props.children = somethingElse. children = somethingElse is perfectly fine since you're not reassigning props.children but the local binding which can be reassigned by spec.

You'd need another lint rule to prevent that. I don't see how removing ReadOnly fixes any of these issues?

I don't see how adding ReadOnly fixes any of these issues?

[jinmayamashita]

@eps1lon Thank you for your review.

I checked your Playground Link and updated the PR to clarify the reason for using Readonly. However, I want to make sure I fully understand your intention.

#67291 (review)
The existing code is fine. The discussion in the issue should highlight this

So, using Readonly is intended to catch props.children = somethingElse, but existing destructuring won't benefit from Readonly, and the current code is fine as it is since children is a local binding?