-
Notifications
You must be signed in to change notification settings - Fork 339
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
Upgrade cosmjs-types to 0.6 🔭 #1329
Conversation
d87bd1b
to
6da40bd
Compare
2fa1602
to
0ccf4cd
Compare
0ccf4cd
to
92b0321
Compare
🎉🎉🎉 very stoked for this! 🙌🏻⚛️ |
We are currently using @webmaster128 was this functionality replaced by something else that I can use for dynamic JSON messages? |
Thank you for bringing this up. The main issue with the old/current fromJSON/toJSON implementations is they are not standardized at all. They do not match the Cosmos SDK JSON representations and as far as I can tell are not greatly following the proto3 JSON convention (but I could be wrong here). So I wonder how useful they can be. Would love to learn more about your use case. |
The main purpose of the wallet is to be very flexible, so devs can test/use anything. A demo is: |
is there a reason not to use Also, there is a potentially to use I'd suggest keeping cosmjs lean, documenting proper usage. For folks that have specific needs, I'm happy to make options in telescope, and transpiler options to cover all their cases. |
For using I think having some JSON import/export is useful, but currently it is underspecified how this JSON is supposed to be structured. The reason for starting #1083 was exactly this: The JSON we get from Cosmos SDK is not a different one than what Type.fromJSON understands. |
what about this: we bring toJSON/fromJSON back to the cosmjs-types build to have a safe and simple baseline set of types. Then we create cosmjs-types-slim to only include the things CosmJS needs internally for signing. This can then (a) have way less types and (b) use a minimal code generation configuration. Then project will either use:
|
Also note, the diff in this PR means toJSON/fromJSON is not required anymore because we don't need them internally. The type generator is still free to have them. |
I'm all on board! This would help us implement cosmjs on osmosis since it can help us start chipping away at bundle size (cc @jonator) also @webmaster128 — should we create a discussion somewhere we can chat about the future "slim" versions? I'm also anticipating the new recursive
I think we have something like this right now
Maybe if we can somehow provide it to proto-signing, or would we need to make a proto-signing-slim?
which could then enable something like
|
For now I'll turn back on the JSON function in cosmjs-types 0.6.1. It's better than nothing and some sort of JSON import/export makes sense to provide. Regarding the bundle size, I think some serious insights are needed first. Since cosmjs-types does not use a central entry point, I'd be surprised if more code than necessary is actually bundled. The number npm shows there includes stuff like source maps, TS type definitions and JavaScript comments that certainly don't make it into the app. Looking at #904, the types are probably not our main concern wrt. size. |
With this upgrade, CosmJS will use types generated by telescope 💪