-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Move to types bundle #4011
Move to types bundle #4011
Conversation
Merge branch 'master' into node-moonbeam-integration-2
Co-authored-by: Jaco Greeff <jacogr@gmail.com>
Co-authored-by: Jaco Greeff <jacogr@gmail.com>
error |
// structs need to be in order | ||
/* eslint-disable sort-keys */ | ||
|
||
export default { |
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 format is slightly different here, and probably the issue.
Here is an example,
export default [
{
// lowest version it applies to, highest version it applies to
minmax: [0, undefined],
// as per normal
types: { ... }
}
]
This allos you to manage multiples, see eg. https://github.com/polkadot-js/api/blob/master/packages/types-known/src/spec/kusama.ts where it is done for Kusama
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.
thanks :)
However, doesn't that mean that the laminar and acala files use the wrong type (OverrideBundleDefinition)?
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.
it looks like using OverrideVersionedType[] like in the example you provided doesn't conform to the type for Api.create of react-api
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.
It may be slightly different, honestly would need to look at what it does under the hood. Too many type options.
(I have been meaning to move all types across for all chains, Acala added the required support into the UI specifically since they had upgrades lines up)
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.
@jacogr have you had the occasion to look? Should I try again?
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.
It should just work as defined here.
You do OverrideVersionedType[]
which aligns with types-known, as first used.
When passed to the API, it expects typesBundle: OverrideBundleType
which is of this form -
typesBundle: {
spec: {
moonbeam: {
rpc?: ... // not sure this is used, don't worry about it now, but if we have a bundle can try
types: OverrideVersionedType[]
}
}
}
The api-react should take the stuff defined and just put it in that format.
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.
Ok, so what you should export is -
export default {
types: OverrideVersionedType[]
};
aka a OverrideBundleDefinition
which looks like -
export interface OverrideBundleDefinition {
alias?: Record<string, OverrideModuleType>;
rpc?: Record<string, Record<string, DefinitionRpc | DefinitionRpcSub>>;
types?: OverrideVersionedType[];
}
TL;DR Seems like you are just missing the top-level types:
key containing the array.
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.
(Like I said once the types is in, we can attempt to move the RPCs as well - not 100% sure the API uses it atm, the intent is there - if it doesn't can fix)
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.
Thanks for the help. Should be ready now!
Like I said once the types is in, we can attempt to move the RPCs as well
Not too hot on adding features that aren't needed explicitly but I can add that as well if you feel this is important
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.
No, I'm sure that stuff is going to break and it is more for my benefit ... so I'll ply with that and see what breaks horribly. However, will get you to review once ready. (Not promising in the next couple of days, but having seen that again makes me want to get it going)
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.
Code looks good.
I am assuming it works for you? Don't want to merge and break :)
yep, ready to merge |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
#4008
From Jaco: "We actually may want to use typesBundle, i.e. versioned types. The reason is then we can do the type overrides (such as this one which will most probably change), based on a version range. (In general, since we have support for it now, it is a better option anyway, since the changes can be made even before the chain gets upgraded and will kick in on upgrade)"