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

Move generation into root #1

Merged
merged 23 commits into from
Jun 2, 2021
Merged

Move generation into root #1

merged 23 commits into from
Jun 2, 2021

Conversation

willclarktech
Copy link
Contributor

@willclarktech willclarktech commented Jun 2, 2021

This PR moves the generation script into the root so the generated files can be reused across different packages. I've left the build step in the package(s). The main open question at this point is about nesting levels for exports from the published packages. For example, from the @cosmjs-types/cosmos package we have types exported from cosmos, gogoproto, google modules etc. Do we want these all at the same level as is the default output from ts-proto?

import { BaseAccount } from '@cosmjs-types/cosmos/cosmos/auth/v1beta1/auth'; // double cosmos is awkward
import { PublicKey } from '@cosmjs-types/cosmos/tendermint/crypto/keys';

Or do we want the cosmos types to be prioritised somehow?

import { BaseAccount } from '@cosmjs-types/cosmos/auth/v1beta1/auth'; // one level of nesting removed
import { PublicKey } from '@cosmjs-types/cosmos/tendermint/crypto/keys';

Or even have the others deprioritised?

import { BaseAccount } from '@cosmjs-types/cosmos/auth/v1beta1/auth';
import { PublicKey } from '@cosmjs-types/cosmos/lib/tendermint/crypto/keys'; // one level of nesting added

This is all assuming we get rid of the build level.

@webmaster128
Copy link
Member

webmaster128 commented Jun 2, 2021

What I would do is spread the output from one source across different packages, e.g.

import { BaseAccount } from '@cosmjs-types/cosmos/auth/v1beta1/auth';
import { PublicKey } from '@cosmjs-types/tendermint/crypto/keys';
import { Any } from '@cosmjs-types/google/protobuf/any';

@webmaster128
Copy link
Member

Or does this mean the cosmos types don't find their dependencies anymore because they are in different packages, e.g. https://github.com/cosmos/cosmjs/blob/main/packages/stargate/src/codec/cosmos/auth/v1beta1/auth.ts#L4

@willclarktech
Copy link
Contributor Author

@webmaster128 Yes exactly, the plan is still to publish separate packages to the extent we need them, but everything here is necessary for the cosmos types only, and given that we encourage the use of nested path imports we have no way of stopping people from importing those other ones.

@webmaster128
Copy link
Member

webmaster128 commented Jun 2, 2021

My plan was to make a direct relation between the proto namespace and the import, i.e. the package name x in @cosmjs/x is the first component of the proto namespace. This does not seem to work. So I wonder if we should publish multiple packages at all or put everything in one package like this:

                         // 👇 no @ here anymore
import { BaseAccount } from 'cosmjs-types/cosmos/auth/v1beta1/auth';
import { PublicKey } from 'cosmjs-types/tendermint/crypto/keys';
import { Any } from 'cosmjs-types/google/protobuf/any';

@willclarktech
Copy link
Contributor Author

In that case, why not add a package to the CosmJS monorepo? @cosmjs/types?

@webmaster128
Copy link
Member

It allows us to remove the ts-proto setup from cosmjs, avoid the re-compilation of the types and have an independent release cycle.

Copy link
Member

@webmaster128 webmaster128 left a comment

Choose a reason for hiding this comment

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

Nice

.gitmodules Outdated
Comment on lines 1 to 3
[submodule "wasmd"]
path = wasmd
url = https://github.com/cosmwasm/wasmd
Copy link
Member

Choose a reason for hiding this comment

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

Does this contain all comos types? I guess we need to prepare for a multi source setup somehow.

Copy link
Member

Choose a reason for hiding this comment

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

Can we rename this folder to wasmd-0.16 and check out at v0.16.0? v0.17.0 contains some .proto changes that are compatible and for wasmd-0.18 we'll need a second source if we want to support the older networks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already checked out at v0.16, but I can rename it to prepare for the multiple sources.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And yes, it contains all the cosmos types. I thought it was neat because we keep the versions in sync, but maybe that won't be sufficient once we have multiple wasmd versions.

Copy link
Member

Choose a reason for hiding this comment

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

It's a good and important start. One thing I would try next is merge cosmos-sdk-0.43 into the output and see what happens. Ideally we only see compatible overrides and additions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll merge this then and add that in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

💪🐎

@willclarktech willclarktech merged commit a50627a into main Jun 2, 2021
@willclarktech willclarktech deleted the centralise-build branch June 2, 2021 13:59
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