-
Notifications
You must be signed in to change notification settings - Fork 121
Mark error context and objects inside those contexts as read-only #1000
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Mark error context and objects inside those contexts as read-only #1000
Conversation
|
6e83a66 to
bbc363a
Compare
| // @ts-expect-error Context is read-only | ||
| e.context.addresses = [] as unknown as typeof e.context.addresses; | ||
| // @ts-expect-error Objects in context are read-only | ||
| e.context.addresses.push('abc' as unknown as (typeof e.context.addresses)[number]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to write a procedural test that looped over every object in SolanaErrorContext to assert that it's read-only, but I couldn't figure out how to. It seems that mutable objects always satisfies Readonly<object>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the same logic that lets us pass Uint8Array to our ReadonlyUint8Array. I think this approach looks fine!
| actualVersion: number; | ||
| }; | ||
| } | ||
| export type SolanaErrorContext = ReadonlyContextValue< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made a little utility to make all of these Readonly.
bbc363a to
eb2a4c2
Compare
BundleMonFiles updated (3)
Unchanged files (127)
Total files change +70B +0.02% Final result: ✅ View report in BundleMon website ➡️ |
eb2a4c2 to
eb0aa0f
Compare
|
Documentation Preview: https://kit-docs-6jhle0l95-anza-tech.vercel.app |
eb0aa0f to
cddbcb1
Compare
| // @ts-expect-error Context is read-only | ||
| e.context.addresses = [] as unknown as typeof e.context.addresses; | ||
| // @ts-expect-error Objects in context are read-only | ||
| e.context.addresses.push('abc' as unknown as (typeof e.context.addresses)[number]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is the same logic that lets us pass Uint8Array to our ReadonlyUint8Array. I think this approach looks fine!
lorisleiva
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! 👌
| expectedAddresses: readonly string[]; | ||
| unexpectedAddresses: readonly string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since these context objects are now automatically readonly-fied, is the convention that we still prefix arrays and object with the readonly keyword? I'm happy either way but I think we should be consistent with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My utility is not recursive. It merely changed this:
type Foo = {
bar: readonly string[],
};to this:
type Foo = {
readonly bar: readonly string[],
};
No description provided.