Skip to content

Conversation

@Lezek123
Copy link
Contributor

@Lezek123 Lezek123 commented Aug 4, 2020

Implements #1118. I created this issue recentlly, so it contains a good summary of the changes (copy-pasting it here). Another, earlier issue explaining some of the reasoning behind the approach I took can also be found here: #1036

This PR includes:

  • Upgrading all Struct and Enum types to use our JoyEnum / JoyStruct wrappers. The reasonsing behind this is that:
    • Had to change all of those anyway, because thier constructors will now take another argument (Registry), so it made sense to use this as an opportunity to perform further normalization.
    • This should greatly simplify implementing similar changes in the future
    • Should also simplify decorating the api with auto-generated type definitions based on runtime metadata (which we can implement at some point, see: https://polkadot.js.org/api/examples/promise/90_typegen)
    • Saves time and code - no need to specify our own getters, since we can just use the ones that come from Struct.with and use one shared TypeScript interface that provides type-safe accessors for all Structs
    • Allows us to preserve a lot of backward compatibility
    • Allows us to possibly introduce some other helpful methods/interfaces for all Structs / Enums (which we use a lot) at once
  • Using Tuple.with(), Vec.with() etc. whenever possible (avoid having to specify constructors on our side for same reasons as mentioned above)
  • Changing some import paths and other minor adjustments related to the removal / renaming of some @polkadot/types
  • Updating all other functions / methods / constructors that now need Registry in order to work
  • Make @joystream/types just export the RegistryTypes that we can then pass as argument when creating the api (since this is now the recommended way of initializing it) instead of registering them "gloablly" with registerJoystreamTypes

Changes required to implement in dependet projects:

  • Instead of using registerJoystreamTypes other project should now initialize the api with:
import { types } from '@joystream/types`
/* ... */
const api = await ApiPromise.create({ provider, types })
  • Passing registry is required as first argument when creating types with new, ie.
new VotingResults(api.registry, {
  abstensions: new u32(api.registry, 0),
  approvals:  new u32(api.registry, 0),
  rejections: new u32(api.registry, 0),
  slashes:  new u32(api.registry, 0),
})

Registry is available in the api instance (api.registry) or any type instance (ie. memberProfile.registry).
This is however not a recommended approach of creating types (see: https://polkadot.js.org/api/start/types.create.html), so I'm considering adding the right augmentation for api.createType so that we can use it instead (in a relatively type-safe way). The problem with api.createTypes() is that it probably won't allow us to force the right schema for stucts like we do currently (ie. previosly some of the struct constructors required a type-safe object with correct Codec values for all struct fields, because the value argument type was set to a corresponding interface). I can think of a workaround using JoyStruct to allow for something like ie. Membership.create(/* typesafe object */) which wouldn't require passing the registry (we could possibly intialize it in @joystream/types itself), but this bypasses the recommended way again, so I'm not sure if that's the right approach..

  • I removed some static methods ie. OptionText.empty() or BlockAndTime.newEmpty(), but I haven't seen any usages of those outside very few cases within Pioneer. Since those would now need the registry in order to work I think I can just replace those usages with a more normalized way of creating types (ie. api.createType, or some new helpers in JoyStruct / JoyEnum as mentioned above)

This is still WIP, but it should be possible to use the library in the current form (although it's not obvious yet how we should approach the creation of types after the upgrade, ie. whether to use api.createType, which will require some additional work, or some other, temporary approach, so I wouldn't advise refactoring other projects yet).

The changes are based on #1071 since it introduced some changes in @joystream/types before. The commit that contains the actual refactorization (most related to this PR) is: c7c531d.

I dind't want to make too much changes just to pass the CI, so I just updated the resolutions in root package.json and temporary disabled storage-node postinstall build, which allows the types checks to pass, but everything else will probably fail (until upgraded)

@Lezek123 Lezek123 marked this pull request as draft August 4, 2020 19:49
@Lezek123
Copy link
Contributor Author

Lezek123 commented Aug 6, 2020

Added some scripts that allow automatically creating augment-types.ts file (related issue: #1127).

api.createType

augment-types.ts file is needed in order to provide the TypeScript interface for api.createType(), so now we can use ie.:

api.createType('Membership', { handle: 'alice' })

And we will:

  • Get the first part ('Membership', auto-suggested, as it has to match an existing type)
  • Get our Codec returned (with TypeSciprt beeing aware of its type)

The idea is based on https://polkadot.js.org/api/examples/promise/90_typegen, but since the tool works with auto-generated interfaces instead of Codec types that we use, I temporary created a modified script.

Using api.createType is the recommended way of creating types in current version of the api.
Unfortunately the second argument it takes doesn't provide an interface for TypeSciprt, so if we want to create the types in a more type-safe way we'd either have to still use our Codec' constructors, ie:

new Membership(api.registry, { /* IMembership */ })

or use:

const membership: IMembership = { /*...*/ }
api.createType('Membership', membership)

It would be nice if @polkadot/api at some point introduced similar augmentation for api.createType as it does for api.tx for example (https://github.com/polkadot-js/api/blob/master/docs/examples/promise/90_typegen/src/interfaces/augment-api-tx.ts) where we can ie. provide u16 argument as u16 | AnyNumber | Uint8Array. If there existed similar (but expanded) interfaces for creating structs (ie. allowing us to skip fields that are optional, provide enums as string literals etc.) that'd be great, but that's not currently available. The good thing about migrating to api.createType() way of creating types though is that it allows us to benefit from any future improvements that may be introduced later. Perhaps using the first approach with new Membership() is also not that bad as of now (in case of Structs and Enums), since we can easily spot those cases by modifying JoyStruct/JoyEnum constructors, but since we should generally use api.createType for other types like u32 etc. I think it's good to stay consistent.

Required setup

In order to use the augmented type definitions other projects need to add:

    "paths": {
      "@polkadot/types/augment": ["path/to/augment-types.ts"]
    }

To tsconfig.json

@joystream/types - creating/updating augment-types.ts

The steps to create the current definitions/augment-types.ts included:

  1. Running yarn workspace @joystream/types generate:defs (currently the definitions we provide are empty, so it just generates augment-types.ts for current version of @polkadot/types)
  2. Removing some yet-unnecessary files like definitions/types.ts
  3. Adding /** CUSTOMIMPORTS **/ and /** CUSTOMTYPES **/ tags in the generated file (see: ecea87a#diff-177b14d3ff2a6665c9746782429b6dcc)
  4. Running yarn workspace @joystream/types update:augment-types which takes care of adding our own types definitions.

Basically steps 1-3 are only needed when we upgrade @polkadot/api / @polkadot/types, if we only update our own types, the last step will take care of everything.

Side-effect benefits

Another benefit we get from generating augment-types.ts is that we can immediately spot any namespace clashes in the types we register. Since the script adds imports for all our types that get registered in the api and aliases them with the name they get registered as ie. import { Lead as LeadOf }, to the file that also contains similar imports for all default @polkadot/types: any flat-namespace related conflicts will result in a TS error in this file!

@Lezek123
Copy link
Contributor Author

Lezek123 commented Aug 6, 2020

One last note: After current refactorization it's possible to use isOfType, asType helpers for all our enums, ie:

  const stageOpt = (await api.query.councilElection.stage()) as Option<ElectionStage>
  const stage = stageOpt.unwrap()
  if (stage.isOfType('Announcing')) {
    console.log('Announcing ends:', stage.asType('Announcing').toNumber())
  }

Also stage.type in that case will have a string literal type allowing for type-safe comparasions, ie.:
stage.type === 'Announcing' (will work)
stage.type === 'ABCDEFG' (TS error, as the condition will never be met)

For all structs we have field getters and helper methods, like:
membership.handle, membership.getField('handle'), membership.getString('handle')
That are type-safe (since all structs use the new, modified JoyStruct)

Unless we previosly had getters that modify the original value (ie. Thread.title and lots of others which I dind't change for temporary backward-compatibility), getters for given struct field return types matching those in the corresponding interface (which means most structs can implement their interface, ie. Membership implements IMembership etc.).
getField on the other hand always returns the original field type, so its a bit more predicatable/consistent currently.

@Lezek123 Lezek123 requested a review from mnaamani August 6, 2020 15:17
@Lezek123 Lezek123 marked this pull request as ready for review August 6, 2020 15:17
@Lezek123 Lezek123 requested a review from gleb-urvanov August 6, 2020 15:17
getField<FieldKey extends keyof FieldTypes>(key: FieldKey): InstanceType<FieldTypes[FieldKey]> {
return this.get(key as string) as InstanceType<FieldTypes[FieldKey]>
}
getString<FieldKey extends keyof FieldTypes>(key: FieldKey): string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest getFieldAsString as a better name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not worth spending more time on this however now, there is more important work.

export class CuratorApplicationIdSet extends Vec.with(CuratorApplicationId) {}

export const contentWorkingGroupTypes = {
ChannelId: 'u64',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unnecessary to specify ChannelId: 'u64' and for the remaining Ids, but this is an artifact from previous version. I can't remember why I was forced to use this style however.

@mnaamani
Copy link
Member

mnaamani commented Aug 7, 2020

Looks good

I tried the creating/updating augment-types.ts steps successfully. It would be better to have a single script that generates both polkadot and the custom types and imports that can be run at anytime without having to think about when is the right time to do it, and avoid having to manually edit the augment-types.ts. (if possible)

@mnaamani
Copy link
Member

mnaamani commented Aug 7, 2020

Merging, noting that dependent packages are broken and will be fixed in coming up PRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants