feat(world): add namespaceLabel to system config #3057
Conversation
🦋 Changeset detectedLatest commit: 9e46c16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| readonly name: string; | ||
| /** | ||
| * Table's resource ID. |
There was a problem hiding this comment.
Like the property descriptions. Just curious is there a reason why it uses single-line comments in packages/cli/src/deploy/common.ts but then multi-line comments here, even for single line comments?
There was a problem hiding this comment.
that file is internal, so not as important to have good docs and those are mostly internal notes
the tsdoc here will get included as part of your IDE tooltips and maybe eventually generated into documentation
| @@ -24,13 +24,39 @@ export type Schema = { | |||
| }; | |||
|
|
|||
| export type Table = { | |||
| /** | |||
| * Human-readable label for this table. Used as config keys, library names, and filenames. | |||
| * Labels are not length constrained like resource names, but special characters should be avoided to be compatible with the filesystem, Solidity compiler, etc. | |||
There was a problem hiding this comment.
Wonder if these special characters should be specified here? I realize "special character" has a pretty universal meaning but it can still be unclear if "-" is allowed, for example. Having said that, curious if there are any other constraints like can a label start with number, etc?
There was a problem hiding this comment.
I don't know the range of special characters in various OS's so I'll leave it to user discretion
if a label has a non-fs compatible character, they'll know quickly because tablegen/worldgen will fail to write files named with those labels
| namespaceLabel: "", | ||
| type: "table", |
There was a problem hiding this comment.
just wonder why only these two properties are used as TABLE_DEFAULTS? Why is namespace not part of, and all the other properties in TableInput type?
There was a problem hiding this comment.
I see now, namespace defaults to namespaceLabel in resolveSystem.
| @@ -171,8 +176,10 @@ export function resolveTable<input extends TableInput, scope extends Scope = Abi | |||
| input: input, | |||
| scope: scope = AbiTypeScope as unknown as scope, | |||
| ): resolveTable<input, scope> { | |||
| const namespaceLabel = input.namespaceLabel ?? TABLE_DEFAULTS.namespace; | |||
| const namespaceLabel = input.namespaceLabel ?? TABLE_DEFAULTS.namespaceLabel; | |||
| // validate ensures this is length constrained | |||
| const namespace = input.namespace ?? namespaceLabel; | |||
There was a problem hiding this comment.
I find it slightly odd that namespace is assigned with namespaceLabel in case it's undefined. This means that namespaceLabel is treated as the default ID. I'd expect it to be in reverse but I suspect it's due to some constraints I'm not aware of yet.
There was a problem hiding this comment.
namespaceLabel is the "entry point" and the strongly typed thing, which propagates to the namespace if it meets namespace criteria (<=14 chars) of the resource ID, otherwise throws in the validate function and requires either adjusting the label or specifying an explicit namespace
| * Human-readable label for this table's namespace. Used for namespace config keys and directory names. | ||
| * Defaults to the nearest namespace in the config or root namespace if not set. | ||
| */ | ||
| readonly namespaceLabel?: string; |
There was a problem hiding this comment.
probably too big of a change now and not worth the effort but wonder if defining namespace as its own type would make sense e.g:
type NamespaceInput = {
id: string;
label?: string;
}
type Table Input = {
namespace?: NamespaceInput;
// ...
}There was a problem hiding this comment.
this would be a breaking change unfortunately
namespaceLabel here won't actually be used by users, it's inferred/propagated from outer context (tables defined inside a namespace in the store/world config)
goes with #3039 and closes some gaps we had there