-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat(world): add types for v2 config resolvers #2389
Conversation
|
1d82bbe
to
546cbec
Compare
9db9060
to
33c99ec
Compare
.eslintrc
Outdated
"@typescript-eslint/ban-types": [ | ||
"error", | ||
{ | ||
"extendDefaults": true, | ||
"types": { | ||
"{}": false, | ||
}, | ||
}, | ||
], |
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.
this is disabling the error when defining a type as "empty 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.
can we add a comment here? I think eslintrc supports it and, if not, we could switch this to .eslintrc.js
https://eslint.org/docs/latest/use/configure/configuration-files#configuration-file-formats
{ | ||
"typescript.tsdk": "node_modules/typescript/lib" | ||
} |
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.
This makes sure vscode uses the repo version of ts instead of a different one in vscode. This is important because some of the type inference only works with the newer versions of typescript, so the dev experience would be much worse if there is accidentally an old version configured
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.
we should add this to the templates too?
@@ -13,7 +13,8 @@ | |||
"esModuleInterop": true, | |||
"forceConsistentCasingInFileNames": true, | |||
"strict": true, | |||
"skipLibCheck": true | |||
"skipLibCheck": true, | |||
"noErrorTruncation": true |
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.
this makes increases the length of types before being truncated, which is useful for development
enums: {}, | ||
namespace: "", | ||
} as const; | ||
attest<typeof expected>(config); |
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.
beautiful
|
||
it("should use the root namespace as default namespace", () => { | ||
const config = resolveStoreConfig({ tables: { Example: {} } }); | ||
attest<"">(config.namespace); |
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.
should we have a test for when namespace is set?
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.
also I thought namespaces were meant to be a world thing, not a store thing?
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.
protocol wise yes, but tablegen also needs namespaces, so it feels fine to add it to the store config (so it can be part of the expected resolved table config)
packages/store/ts/config/v2/store.ts
Outdated
: key extends "userTypes" | ||
? UserTypes | ||
: key extends "enums" | ||
? narrow<input[key]> | ||
: input[key]; | ||
}; | ||
|
||
export type resolveEnums<enums> = { [key in keyof enums]: Readonly<enums[key]> }; |
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.
do we have a rule for when we use readonly
keys vs not? (I don't see one here on enum keys, for example)
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.
Should probably always be readonly
. I think the test is passing in this case because of the const input
parameter, so the incoming enum config is already readonly, which makes the enum object on the output readonly too. Will still add it here for consistency.
}>; | ||
|
||
export function resolveStoreConfig<input>(input: validateStoreConfig<input>): resolveStoreConfig<input> { | ||
export function resolveStoreConfig<const input>(input: validateStoreConfig<input>): resolveStoreConfig<input> { |
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.
does this make it so a regular object input gets used as if it were a const/strongly typed?
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.
namespace: "", | ||
} as const; | ||
|
||
attest<typeof expected>(config); |
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.
the extra line break above is helpful for readability, wouldn't mind if all of these attest statements had this
}, | ||
Second: { | ||
schema: { secondKey: "address", secondName: "string", secondAge: "uint256" }, | ||
// @ts-expect-error Type '"firstKey"' is not assignable to type '"secondKey" | "secondAge"' |
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.
not at all blocking, but where this condition lives, do we have room to expand on/improve this error type?
like a branch that outputs one of the following:
firstKey
field was not found in the schema
firstKey
field is not a static ABI type
either of those feel more actionable to me than "this item is not one of this union of items", but I also haven't been working with these types a ton to say if this is actually better
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.
this one is a native typescript error, it's probably possible to create a validator function that routes this condition to a custom string error, but would leave that for later
Second: { | ||
schema: { secondKey: "address", secondName: "string", secondAge: "uint256" }, | ||
// @ts-expect-error Type '"firstKey"' is not assignable to type '"secondKey" | "secondAge"' | ||
primaryKey: ["firstKey", "secondAge"], |
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.
do we have/want/need any checks around "unique fields" defined here so we don't allow for ["firstKey", "firstKey"]
?
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.
Don't have it yet, unclear if it's possible at a type level, but will ask david about ideas. Maybe we can map the tuple to an object that has the tuple values as keys, then convert it back to a tuple and check the length? Would also keep this one for later though.
packages/world/ts/config/v2/world.ts
Outdated
|
||
export type NamespacesInput = { [key: string]: NamespaceInput }; | ||
|
||
export type NamespaceInput = Omit<StoreConfigInput, "userTypes" | "enums">; |
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.
do you think this will be annoying to maintain vs the inverse? Pick<StoreConfigInput, "tables">
?
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.
agree it might be nicer to explicitly pick the things we want there than adding a store config option and forgetting about it here
? { | ||
readonly [key in namespacedTableKeys<input>]: key extends `${infer namespace}__${infer table}` | ||
? resolveTableConfig< | ||
get<get<get<get<input, "namespaces">, namespace>, "tables">, table>, |
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.
🙈
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.
yeah haha, there might be a prettier way to do this with some kind of deepGet
type that takes a tuple of keys but i opted against it for the moment bc 1. don't have a deepGet
type yet and 2. it might be the case that this manual nesting here takes better advantage of the typescript cache (can only really know that once we do some type benchmarking)
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.
looks great! mostly just questions/curiosities, nothing blocking
No description provided.