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

feat: re-enable TS codegen #3655

Merged
merged 42 commits into from
Oct 24, 2023
Merged

feat: re-enable TS codegen #3655

merged 42 commits into from
Oct 24, 2023

Conversation

clockworkgr
Copy link
Collaborator

This was supposed to be a simple code reversion but ended up being much more.

Final list of steps taken:

  • Module Resolution algorithm updates:
    We now have a single resolveIncludes() method which takes a module path and returns a list of proto include folders needed to build this module. It does so by including default protoc include folders, the module's own proto folder, the app's proto include folders and if module is using buf, it uses buf export to gather required dependencies. If not it simply adds the standard third_party protoDirs

  • cosmosgen generator struct updates:
    appModules now has a corresponding appIncludes field which contains the output of resolveIncludes for the app's modules. Similarly, each entry in thirdModules is now paired by its corresponding includes field.

  • swagger-typescript-api bumped down to last known working version. Update will happen in another PR

  • protoc-based openap[i generation scrapped due to incompatibility with current proto structures. Switched to the newly introduced buf version instead. Added a method to generate a merged openapi spec for a single module without operation id renaming to match the prior codegen behaviour

  • pegged TS version in nodetime to last known working as more recent minor TS updates are incompatible with sta

Things to note:

  • The changes in swagger-combine and generate_openAPI def need review. Former was done as a quick fix. Latter is mostly duplicated, should be refactored into a single configurable call.
  • Hit the same issue as: https://github.com/ignite/cli/blob/main/ignite/pkg/cosmosbuf/buf.go#L91-L95 Wasn't sure how to fix and since sdk protos are already included in the defautl app includes I simply ignore the cosmos-sdk module.

@clockworkgr
Copy link
Collaborator Author

Also, This is not quite draft nor quite ready.

It should be added to based on feedback

@clockworkgr
Copy link
Collaborator Author

clockworkgr commented Sep 13, 2023

Just realised this STILL won't work due to: https://github.com/ignite/cli/blob/main/ignite/pkg/cosmosbuf/buf.go#L91-L95

Normally the buf export there would also export the (required) google/protobuf/any , google/protobuf/duration, google/protobuf/timeout protos.

Need to add them manually or simply reference them in our own proto files (empty proto with 3 import statements should do it)

Any other ideas welcome...Although I think the best way is to figure out how to get around the orm/internal buf error.

clockworkgr and others added 3 commits September 13, 2023 22:49
…#3657)

* feat(pkg/protoc): change package to use protoc binary from files repo

* chore(pkg/protoc): remove embedded protoc binary
jeronimoalbi
jeronimoalbi previously approved these changes Oct 19, 2023
@Pantani Pantani self-requested a review October 19, 2023 14:55
go.mod Show resolved Hide resolved
ignite/pkg/cosmosbuf/buf.go Outdated Show resolved Hide resolved
ignite/pkg/cosmosbuf/buf.go Outdated Show resolved Hide resolved
ignite/pkg/cosmosbuf/buf.go Outdated Show resolved Hide resolved
ignite/pkg/cosmosbuf/buf.go Show resolved Hide resolved
ignite/pkg/cosmosgen/generate.go Outdated Show resolved Hide resolved
ignite/pkg/cosmosgen/generate_typescript.go Outdated Show resolved Hide resolved
ignite/pkg/cosmosgen/generate_typescript.go Show resolved Hide resolved
ignite/services/scaffolder/scaffolder.go Show resolved Hide resolved
ignite/pkg/cosmosgen/generate_openapi.go Outdated Show resolved Hide resolved
@Pantani Pantani added this to the v0.28 milestone Oct 19, 2023
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

❗ No coverage uploaded for pull request base (main@50bf90d). Click here to learn what that means.
The diff coverage is 0.39%.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3655   +/-   ##
=======================================
  Coverage        ?   24.71%           
=======================================
  Files           ?      289           
  Lines           ?    23945           
  Branches        ?        0           
=======================================
  Hits            ?     5918           
  Misses          ?    17494           
  Partials        ?      533           
Files Coverage Δ
ignite/pkg/cosmosanalysis/app/app.go 65.87% <100.00%> (ø)
ignite/pkg/xos/files.go 82.35% <ø> (ø)
ignite/pkg/gomodule/gomodule.go 23.07% <50.00%> (ø)
ignite/services/scaffolder/scaffolder.go 0.00% <0.00%> (ø)
ignite/services/scaffolder/init.go 0.00% <0.00%> (ø)
ignite/pkg/cosmosgen/generate_composables.go 0.00% <0.00%> (ø)
ignite/pkg/cosmosgen/generate_vuex.go 0.00% <0.00%> (ø)
...detime/programs/swagger-combine/swagger-combine.go 0.00% <0.00%> (ø)
ignite/services/chain/generate.go 0.00% <0.00%> (ø)
ignite/pkg/nodetime/programs/ts-proto/tsproto.go 0.00% <0.00%> (ø)
... and 6 more

@Pantani Pantani merged commit f643ccd into main Oct 24, 2023
26 checks passed
@Pantani Pantani deleted the chore/revert-ts-codegen branch October 24, 2023 22:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants