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

web: add generators for typescript defs #2422

Merged
merged 1 commit into from
Oct 24, 2019
Merged

Conversation

nicks
Copy link
Member

@nicks nicks commented Oct 24, 2019

Hello @jazzdan,

Please review the following commits I made in branch nicks/web-grpc-typescript:

4f5b3ac (2019-10-24 18:02:06 -0400)
web: add generators for typescript defs

@nicks nicks requested a review from jazzdan October 24, 2019 22:02
@nicks nicks force-pushed the nicks/web-grpc-typescript branch from f69f879 to 4f5b3ac Compare October 24, 2019 22:03
@@ -0,0 +1,22 @@
declare namespace OpenAPI2 {
export interface webviewView {
log?: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the main thing to look at

separately, we should strategize on how to substitute these generated types for the handwritten types in types.ts

Copy link
Contributor

Choose a reason for hiding this comment

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

Aww, it makes all of these optional fields? Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

in proto3, fields can only be optional. there's some good discussion here:
protocolbuffers/protobuf#2497
https://capnproto.org/faq.html#how-do-i-make-a-field-required-like-in-protocol-buffers

@nicks nicks force-pushed the nicks/web-grpc-typescript branch from 4f5b3ac to f626c75 Compare October 24, 2019 22:06
toast.yml Outdated
output_paths:
- web/src/view.d.ts
command: |
npx @manifoldco/swagger-to-ts pkg/webview/view.swagger.json --camelcase --output web/src/view.d.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to continue with using yarn everywhere I think we can change this to use yarn dlx:

Suggested change
npx @manifoldco/swagger-to-ts pkg/webview/view.swagger.json --camelcase --output web/src/view.d.ts
yarn dlx @manifoldco/swagger-to-ts pkg/webview/view.swagger.json --camelcase --output web/src/view.d.ts

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@nicks nicks force-pushed the nicks/web-grpc-typescript branch from f626c75 to 6c1d647 Compare October 24, 2019 22:20
@nicks nicks merged commit 01c7c3a into master Oct 24, 2019
@nicks nicks deleted the nicks/web-grpc-typescript branch October 24, 2019 22:37
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