Comments (7)
I don't see how adding ReadOnly fixes any of these issues?
Maybe this helps illustrating it: Playground Link
from next.js.
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 }) {
// ...
}
from next.js.
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.
from next.js.
@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.
from next.js.
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?
from next.js.
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 catchprops.children = somethingElse
.children = somethingElse
is perfectly fine since you're not reassigningprops.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?
from next.js.
@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?
from next.js.
Related Issues (20)
- Failed to complie and load Server, SyntaxError: Invalid regular expression HOT 2
- Turbopack can't locate serverExternalPackages if installed via pnpm and are child dependencies of other installed modules HOT 19
- Test
- Create a bug report for Next.js. If this doesn't look right, choose a different type. HOT 1
- Standalone Output doesn't work
- Top Suspence fallback not work in chrome or android
- Cached function in layout and page runs twice
- Default hostname for "next start" differs from documentation
- Docs: Migration from vite shows app router docs HOT 1
- How to use generated "Route" type (from "typedRoutes") with dynamic routes
- Enabling Draft Mode Not Working in Next.js 14.2.5
- Re-exported modules from index.ts are not tree shaken
- v14 npm run lint (eslint+typescript) Not implemented correctly HOT 3
- `app/icon.tsx` and `npm run dev` with `output: "export"` HOT 2
- Next.js App Directory: Link Component Crashes on First Load with Storyblok Integration
- Docs: learn -> improving-accessibility, missing something about accessiblity HOT 1
- postcss.config.js not work with type module
- Use intercepting and parallel route with root level not working HOT 1
- Next.js downloads from fonts.googleapis.com during build HOT 1
- Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from next.js.