refactor!: migrate api package to use telescope #68
Conversation
|
Great work here @ryanchristo I think when testing this, it would be ideal to look at the generated bundle file simply because this adds a fair amount of code. One of the downsides with telescope is that it doesn't handle code-splitting very well, and while the majority of bundle is through 3rd party libs we can't do much about (libsodium and some crypto libs), this could potentially have perf impact on the registry app if it bloats regen-js more than it already is. I think to test would require:
|
| @@ -50,7 +50,7 @@ message GetTxsEventRequest { | |||
| // pagination defines a pagination for the request. | |||
| // Deprecated post v0.46.x: use page and limit instead. | |||
| cosmos.base.query.v1beta1.PageRequest pagination = 2 [deprecated = true]; | |||
| ; | |||
|
|
|||
There was a problem hiding this comment.
extra semicolon was causing telescope to fail
| @@ -16,7 +16,7 @@ service Query { | |||
| rpc AnchorByIRI(QueryAnchorByIRIRequest) returns (QueryAnchorByIRIResponse) { | |||
| option (google.api.http) = { | |||
| get : "/regen/data/v1/anchor-by-iri/{iri}" | |||
| additional_bindings : [ {get : "/regen/data/v1/anchors/iri/{iri}"} ] | |||
| // additional_bindings : [ {get : "/regen/data/v1/anchors/iri/{iri}"} ] | |||
There was a problem hiding this comment.
Telescope was returning unexpected [ character error. Additional bindings are not necessary.
There was a problem hiding this comment.
does this mean we'll need to comment those manually as long as there's not a fix on telescope side?
There was a problem hiding this comment.
Correct. Eventually we shouldn't have to make any changes to the proto files and we could use buf to pull the proto files from buf registry and gitignore proto files. This would require adding a fix on telescope though as well as fixing the proto file with the extra ; (or adding a fix on telescope for this as well).
| @@ -0,0 +1,6 @@ | |||
| export const AMINO_MAP = { | |||
| // TODO: add amino types | |||
There was a problem hiding this comment.
We should create a separate issue once merged to track adding amino support.
| @@ -14,7 +14,7 @@ | |||
| "build": "lerna run build --stream", | |||
| "format": "yarn lint --fix", | |||
| "lerna:deploy": "yarn build && lerna version --conventional-commits && lerna publish from-git", | |||
| "lint": "tsc --noEmit && eslint --ext js,ts,tsx .", | |||
| "lint": "eslint --ext js,ts,tsx .", | |||
There was a problem hiding this comment.
Currently there are typescript errors in the generated code. Removing ts compile from the lint script for now but we may want to add back when we have a solution.
| @@ -1,19 +0,0 @@ | |||
| syntax = "proto3"; | |||
|
|
|||
| package ibc.applications.transfer.v1; | |||
There was a problem hiding this comment.
The latest proto from the ibc repository was causing errors when generating code due to an import that does not exist. I think I found the right file but received additional errors. I did not update ibc proto for now and maybe best to address as a separate issue.
There was a problem hiding this comment.
the issue is that we do use ibc in the registry app, in particular https://buf.build/cosmos/ibc/docs/main:ibc.applications.transfer.v1#ibc.applications.transfer.v1.Query.DenomTrace and https://buf.build/cosmos/ibc/docs/main:ibc.applications.transfer.v1#ibc.applications.transfer.v1.Query.DenomTraces
There was a problem hiding this comment.
I can add back the proto files that are in the current api package and then maybe we can address updating the ibc proto files in a separate issue. There are probably a few issues we need to resolve before this new version of the api package is ready for regen-web but hopefully those won't be too difficult to address in followups.
Closes: regen-network/groups-ui#33
This pull request migrates the
regen-tscode toregen-jswithin a new packageapi-v1and fixes the reported error. As far as I understand, this will eventually replace the need for the existingapipackage when we are ready to migrate.An alternative to adding this as a new package would be creating a release branch (e.g.
release/v0.x) and replacingapion themainbranch. This would be my preference, which would allow us to use package nameapi(otherwise we would need to change the package name withinpackage.jsonfor publishing releases) and we would be able to continue maintaining thev0series by merging fixes into the release branch and publishing from there.Note: Updated to use alternative approach mentioned above ☝️
Credit to @haveanicedavid for getting this work started in: https://github.com/haveanicedavid/regen-ts