-
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(cli): use abi types in store config #507
Conversation
packages/cli/src/config/index.ts
Outdated
export type MUDConfig = StoreConfig & ResolvedWorldConfig; | ||
|
||
/** Type helper for defining MUDUserConfig */ | ||
export function defineMUDUserConfig<EnumNames extends StringForUnion = StringForUnion>( |
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.
nit: would be great if we could just call this mudConfig({...})
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.
For consistency we should probably capitalize MUD
here.
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 storeConfig
be StoreConfig
then to consistently capitalize them too?
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.
disagree! would much rather we loosen how/when we capitalize MUD than change context-meaningful capitalization of code (i.e. types/components vs variables/functions), otherwise it's gonna be really confusing
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.
doing a little refactoring PR rn, capitalized func names don't feel good. And with MUDConfig I also get conflicts with types and zod stuff
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.
defineMUDConfig
would be a better alternative, but I'd still prefer mudConfig
as it's shorter
name, | ||
memberNames: config.userTypes.enums[name], | ||
memberNames: config.enums[name], |
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 allows unions between string literals and `string` without sacrificing autocompletion. | ||
// Workaround for https://github.com/Microsoft/TypeScript/issues/29729 | ||
export type StringForUnion = string & Record<never, never>; |
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!
x: SchemaType.UINT32, | ||
y: SchemaType.UINT32, | ||
x: "uint32", | ||
y: "uint32", |
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.
ohhh this is sooo much 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.
just one small nit about simplifying the name the config function as we gear up towards release!
@@ -151,22 +169,29 @@ export interface StoreUserConfig { | |||
* The key is the table name (capitalized). | |||
* | |||
* The value: | |||
* - `SchemaType | userType` for a single-value table (aka ECS component). | |||
* - abi or user type for a single-value table (aka ECS component). |
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 part of this PR, but I think we should remove aka ECS component
. Having a single value is not the distinguishing factor for an ECS component, since components can have multiple (related) values (eg. PositionComponent = {x: int32, y: int32 }
). The distinguishing factor for ECS components vs regular tables would rather be having a single key vs a composite key.
uint56: RecsType.Number, | ||
uint64: RecsType.Number, | ||
uint72: RecsType.Number, | ||
uint80: RecsType.Number, | ||
uint88: RecsType.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.
Is anything above uint53
really represented as number
in recs
? Since those don't fit into javascript number
and recs
in its current state doesn't support bigint
, I would expect them to be represented as a hex string (= RecsType.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.
Defer to @holic (this isn't really a part of this PR, as I just refactored the previous mapping of schema types)
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 RECS consistently give us back a native number? iirc some things use BigNumber
under the hood
Config now uses strings for both abi and user types
I also added a function to define configs, because otherwise user types can't really be typehinted
When using the function any missing user types will show a typescript error
(to simplify the logic for this I also removed
userTypes
, soenums
are in config root now)