Skip to content
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

fix: augmented type generation #747

Merged
merged 6 commits into from
Apr 17, 2023
Merged

fix: augmented type generation #747

merged 6 commits into from
Apr 17, 2023

Conversation

rflechtner
Copy link
Contributor

@rflechtner rflechtner commented Apr 13, 2023

fixes KILTProtocol/ticket#2576

We're not the only ones with that problem, and from issues on the polkadot-js/api repo I took that type augmentation can no longer be run in cjs. We are now required to add the --esm flag when calling ts-node and to add the "type":"module" to the package.json. Fearing that this may interfere with the dual cjs/esm code generation in our repo, and realising we do not actually need code, just types from the augment-api package, I have refactored the build process only of this package to just produce types (and a few dummy files required for importing them from different environments.

Along the way, I have also tackled two FIXMEs that we've had sitting there for a while now.

How to test:

I tried building and publishing with yalc and using the resulting package in stakeboard, which works fine. Could be tried in other projects using this package as well.

Checklist:

  • I have verified that the code works
  • I have verified that the code is easy to understand
    • If not, I have left a well-balanced amount of inline comments
  • I have left the code in a better state
  • I have documented the changes (where applicable)

@rflechtner rflechtner requested a review from ntn-x2 April 13, 2023 17:17
Copy link
Member

@ntn-x2 ntn-x2 left a comment

Choose a reason for hiding this comment

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

Not too deep into all these JS/TS stuff, but it looks good. The only thing is that if I run yarn build in the root and then yarn build:types from within the augmentation package, the compiler still complains that src/interfaces/extraDefs/types.ts cannot find the name PalletDidLookupLinkableAccountLinkableAccountId, and in the diff I can see the old FIXME is gone. Is that expected? Is it just my own VSCode compiler issue?

@@ -275,7 +275,7 @@ declare module '@polkadot/rpc-core/types/jsonrpc' {
/**
* Returns the number of transactions sent from given address at given time (block number).
**/
getTransactionCount: AugmentedRpc<(hash: H256 | string | Uint8Array, number?: BlockNumber | AnyNumber | Uint8Array) => Observable<U256>>;
getTransactionCount: AugmentedRpc<(address: H160 | string | Uint8Array, number?: BlockNumber | AnyNumber | Uint8Array) => Observable<U256>>;
Copy link
Member

Choose a reason for hiding this comment

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

Is this auto-generated? Why do we have a change from AccountId32 to AccountId20 here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah we haven't been able to run type generation on this polkadot version, so this has been outdated until now

Copy link
Member

Choose a reason for hiding this comment

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

I get a linter error that the header is missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

weirdly, I don't ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added the header nevertheless

Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

@rflechtner
Copy link
Contributor Author

The only thing is that if I run yarn build in the root and then yarn build:types from within the augmentation package, the compiler still complains that src/interfaces/extraDefs/types.ts cannot find the name PalletDidLookupLinkableAccountLinkableAccountId, and in the diff I can see the old FIXME is gone. Is that expected?

It's 'expected' - that we once introduced a manual fix to an autogenerated file means that we'll have to reapply this fix every time after autogeneration, which in most cases just means reverting the change to this line.

@rflechtner rflechtner requested a review from ntn-x2 April 17, 2023 09:48
@rflechtner rflechtner merged commit f121892 into develop Apr 17, 2023
@rflechtner rflechtner deleted the rf-fix-type-generation branch April 17, 2023 13:26
rflechtner added a commit that referenced this pull request May 24, 2023
* fix: api-augment type generation
* fix: extraDefs subpath export
* chore: run type generation
* chore: add explanatory comments
@rflechtner rflechtner mentioned this pull request Jun 8, 2023
rflechtner added a commit that referenced this pull request Jul 26, 2023
* fix: api-augment type generation
* fix: extraDefs subpath export
* chore: run type generation
* chore: add explanatory comments
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