-
-
Notifications
You must be signed in to change notification settings - Fork 15
feat: named CAIP structs #228
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
Conversation
c6f90b3 to
3518871
Compare
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
| export const CaipChainIdStruct = definePattern<`${string}:${string}`>( | ||
| 'CaipChainId', | ||
| CAIP_CHAIN_ID_REGEX, | ||
| ) as Struct<CaipChainId, null>; |
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.
In the PR description you say that the struct is being inferred as a string, but it looks like we were using a type assertion before to ensure that this was not the case. So, was it really being inferred as a string? It's not clear to me what effects these changes have, as the only tests being added in this PR are for definePattern and not for any of the structs.
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.
pattern(string(), ...) would result in a type like this Struct<string, ...>
Meaning that using Infer<typeof MyType> would be inferred as string.
With the new definePattern helper I've added, we now uses a generic type to inject it into the Struct like so:
export const CaipChainIdStruct = pattern(
string(),
CAIP_CHAIN_ID_REGEX,
); // Would be: Struct<string, null> (which is why we were type-casting)
export const CaipChainIdStruct = definePattern<`${string}:${string}`>(
'CaipChainId',
CAIP_CHAIN_ID_REGEX,
); // Would be: Struct<`${string}:${string}`, null> (no more type-casting required)In addition to that, we use define under the hood, which allows use to have a "named" struct (and better error messages).
b1b5d70 to
8969892
Compare
87c6bb0 to
a920704
Compare
Co-authored-by: Daniel Rocha <68558152+danroc@users.noreply.github.com>
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
|
|
Currently, all our CAIP structs are being inferred as
string. This PR adds a newdefinePatternwhich allows to using a template literal string instead.Also, this new
definePatternallows to "name" thepattern. Here's an example:If you think the new
definePatternshould go in a separate PR, I can split that up.I do believe this is BREAKING CHANGE since the error messages will be different, so we can expect the consumers of this packages to update some of there tests (but I don't really know if we consider this a breaking change in other packages?)