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

Generate Typescript definition for top level Enums #404

Merged
merged 6 commits into from
Dec 7, 2018

Conversation

rogchap
Copy link
Contributor

@rogchap rogchap commented Dec 3, 2018

Fixes #403

@stanley-cheung
Copy link
Collaborator

stanley-cheung commented Dec 4, 2018

Thanks for the great contributions! Yea looks like this is a gap that we need to address.

On the other hand, I'd like to start requesting contributors to also add tests for features they added. We already have some tests - running npm test from the packages/grpc-web directory will trigger those.

This PR will fit in to something like https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/test/plugin_test.js. In that test, we assume the protoc-gen-grpc-web is already installed somewhere in the system (in reality, before npm test is run, we will re-compile the plugin every time a PR is updated), then we run the protoc ... --grpc-web_out=... command to actually generate the client stub (you can make up a new .proto file highlighting the Enum change), load the generated stub file via a require() statement and do some test on the client stub class.

Let's do something similar and add a test for this Enum feature.

Thanks!

@rogchap
Copy link
Contributor Author

rogchap commented Dec 4, 2018

@stanley-cheung Totally agree that we should have tests for this.
Hard to load the file via require() as the JS is untouched and it's the TypeScript generation that we need to test.
Do you have any advise on how to test the TypeScript generation; my best thought is doing a string comparison/regex on the file, but that seems ugly.

@stanley-cheung
Copy link
Collaborator

I am also hoping someone from the community can give us some advice on how's best to test TS code generation.

Quick thought: perhaps we can exec('tsc')? At least that should weed out some TS compilation errors. I am actually not sure what's the "standard" way of testing TS code in a npm package these days. Also kinda new to this :)

@johanbrandhorst
Copy link
Collaborator

Looking at the TS protoc generator Improbable has written, it seems they generate the files and just check properties on the generated types: https://github.com/improbable-eng/ts-protoc-gen/blob/master/test/integration/service/grpcweb.ts.

@rogchap
Copy link
Contributor Author

rogchap commented Dec 4, 2018

From what I understand this would still just test the properties on the compiled JavaScript and not the actual TypeScript file itself; it just looks that way because the test itself is in TypeScript and can import the definition.

@johanbrandhorst
Copy link
Collaborator

I plead ignorance, I just wanted to point to the Improbable repo as a possible source for inspiration for writing tests for this repo :).

@rogchap
Copy link
Contributor Author

rogchap commented Dec 4, 2018

So... I've added a basic test to make sure the TypeScript can compile. I manually tested tsc to see if it would fails on a malformed typescript definition file, which it does; so at least we know the TypeScript is right as far as it can compile.

The thing is, we can't test any output from the TypeScript to validate as this is only a definition file and the actual JS is already generated 💁‍♂️.

Copy link
Collaborator

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for adding the tests!

@stanley-cheung stanley-cheung merged commit 1bbfb21 into grpc:master Dec 7, 2018
@pacurtin
Copy link

Awesome work you guys are doing. I'm currently using JS web-grpc but I'm looking forward to switching to TS. I'm waiting for this fix to be released so I can do so.

Sorry if this is the wrong place to ask but do you know when the next release is scheduled for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TS declarations files are missing enums
5 participants