Skip to content

Conversation

@amarts
Copy link
Contributor

@amarts amarts commented Dec 1, 2020

If a pallet is updating a block with type Object (Record<string, string>),
there is currently no way to provide a structure/Object interface type.
Enhancing the type of the CUSTOM_TYPES to take json object with
Record<string, string> type would fix the issues.

Signed-off-by: Amar Tumballi [email protected]

@amarts amarts force-pushed the custom_type_enhance branch from 809e4fd to 6e2b688 Compare December 1, 2020 05:28
@amarts
Copy link
Contributor Author

amarts commented Dec 1, 2020

Looks like this has issues with yarn build. Things worked for me when I have Record<string, any> | undefined as the type, but yarn lint warns about using 'any' as the type (well, I too don't like it).

As I am not an expert with TypeScript yet, would like to get advise on how to handle a object in the config/types.json file. Thanks in advance.

@emostov emostov self-requested a review December 1, 2020 16:32
@emostov emostov added the B0 - Silent PR should not be mentioned in release notes label Dec 1, 2020
@emostov
Copy link
Contributor

emostov commented Dec 1, 2020

Thank you for the PR! As I mentioned in the code comment, I think AnyJson from polkadot-js would be a good type to use here. I also can finish up this PR if its ok with you.

You might also be interested in #351, as that makes sidecar much more friendly to non polkadot/kusama substrate based chains by pulling in types from @polkadot/apps-config and allowing chain builders to config which routes to mount.

If a pallet is updating a block with type Json, the Record<string, string>
is not going to meet the needs.
Enhancing the type of the CUSTOM_TYPES to take json object with
`AnyJson` type would fix the issues.

Signed-off-by: Amar Tumballi <[email protected]>
@amarts amarts force-pushed the custom_type_enhance branch from 6e2b688 to 09585fb Compare December 2, 2020 16:02
@amarts
Copy link
Contributor Author

amarts commented Dec 2, 2020

Sure. #351 looks good. Will keep a watch.

meantime, did a --force push even though CONTRIBUTE guide suggests otherwise mainly to fix the commit message to reflect the actual change.

@emostov
Copy link
Contributor

emostov commented Dec 2, 2020

Looks like CI is failing because we can't assume AnyJson is an object. To get around that I think we could do Record<string, AnyJson>

@amarts
Copy link
Contributor Author

amarts commented Dec 2, 2020

Even that gives error:

amar@bulde:~/work/dhiway/substrate-api-sidecar$ yarn lint --fix
yarn run v1.22.0
$ tsc && eslint . --ext ts --fix
src/main.ts:60:3 - error TS2322: Type '{} | { [x: string]: AnyJson; }' is not assignable to type 'Record<string, string | Record<string, string> | Constructor<Codec> | { _enum: string[] | Record<string, string | null>; } | { _set: Record<string, number>; }> | undefined'.
  Type '{ [x: string]: AnyJson; }' is not assignable to type 'Record<string, string | Record<string, string> | Constructor<Codec> | { _enum: string[] | Record<string, string | null>; } | { _set: Record<string, number>; }>'.
    Index signatures are incompatible.
      Type 'AnyJson' is not assignable to type 'string | Record<string, string> | Constructor<Codec> | { _enum: string[] | Record<string, string | null>; } | { _set: Record<string, number>; }'.
        Type 'undefined' is not assignable to type 'string | Record<string, string> | Constructor<Codec> | { _enum: string[] | Record<string, string | null>; } | { _set: Record<string, number>; }'.

60  	types: {
    	~~~~~


Found 1 error.

@amarts
Copy link
Contributor Author

amarts commented Dec 2, 2020

@emostov as you mentioned #351 fixes the issue in much cleaner way, I am happy to wait for that to get merged and released. It is ok to not spend time on this if it gets changed soon anyways.

Feel free to close this in that case.

@emostov
Copy link
Contributor

emostov commented Dec 2, 2020

I pushed directly to the branch. Not sure why I didn't think of it earlier, but just pulled the exact type polkadot-js uses for the object - RegistryTypes. I think it should be all good to go now.

@emostov emostov assigned insipx and unassigned insipx Dec 2, 2020
Copy link
Contributor Author

@amarts amarts left a comment

Choose a reason for hiding this comment

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

Thanks, Working for me :-)

@emostov emostov merged commit 692193d into paritytech:master Dec 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B0 - Silent PR should not be mentioned in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants