-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 type definitions #1273
Generate TypeScript type definitions #1273
Conversation
…or writing to stream Splits the definition generator into two submodules, one for parsing the externs and one for writing the definition file.
…inputs Moves all function related to building the definition tree into a separate submodule. Furthermore, the entry-function in generateTypescriptDefinitions now accepts multiple input files and merges their definitions into one tree.
- Rename property indentationLevel of AbstractWriter class to just level to show it can be used for more than one purpose - Mark namespaces at level 0 as ambient by using the declare keyword
Output normal type aliases as `type` declarations and type object strucutes as interfaces
Previously, the type expression was passed as-is from the `@implements` keyword. Now directly passes the name and throws if implements is not followed by a name expression.
If a class node's attributes define a base class and/or interface, append them to the class declaration when generating the output for the class node.
…egation - Move type generation to separate module as it will get a bit bigger as well - Fix a bug in the aggregation of static class members. The attribute type was ignored here.
- Implement python method generate_typescript_definition as part of the build process - Call generateTypescriptDefinitions from python with the generated externs and all externs/shaka scripts - Adjust entry of generateTypescriptDefinitions to match generateExterns
Shaka player classes that implement an interface with properties just specify the property with the @OverRide tag. TypeScript requires implemented properties to specify the type again, so we have to infer the types from the interface that is implemented.
This is required for TypeScript to automatically pick up the typing if shaka-player is imported from npm package
…teTypeDefinitions
I have fixed a remaining issue in how the parameter types were inferred from interfaces, but that is done as well now. I think the pull request is ready for a review and feedback now. @joeyparrish |
The generated externs did not include shaka.polyfill because we used the wrong annotation on the class. This fixes it. Raised in PR #1273 Change-Id: I348064a117a7e1878b439ad8bd1ce49df56bfd39
…feature/generate-typescript-typedefs
I've updated the branch to shaka-player v2.5.8 and published an according build at |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some TS files in your PR which are too large to have their diff displayed on github. That concerns me.
@joeyparrish Thanks a lot for taking the time to review! I have already resolved some of your remarks and I will fix the issue with linebreaks in type annotations. As for the other three comments, I'll await your response so we can decide how to proceed there. |
I have fixed all remaining issues. Please feel free to re-open a discussion if you don't consider it resolved @joeyparrish. Also, I have fixed a few syntax errors in df0700b |
…typescript-typedefs
I've updated the repository to the current master but I'm experiencing some issues regarding the newly introduced use of iterators. I will have a look at this in the coming week. |
What is stopping this PR to be merged? |
@CHaNGeTe I have yet to adapt the code to shaka-player 3 Edit to clarify: in general, no changes are needed when shaka-player is being updated, but if new syntactic elements are used for defining classes, functions etc (which happened quite a lot in the last few months because the shaka-player code base is making more and more use of ES2015+, as opposed to almost ES5 only when I started writing the generator), the parser has to be updated so it knows where to get the necessary type information from. |
I'm sorry for not giving you this feedback sooner, but I dislike that your approach involves an additional parser. We already have a parser that supports the whole codebase in I would prefer an approach where we create two outputs from that one parser and single pass over the input, converting the closure type annotations at that time. If we end up tackling this ourselves, that is the approach we would take. |
@joeyparrish I get your point of view and appreciate the honesty. I understand the additional maintenance overhead coming with the parser. However, at this point I'm not willing to invest any further resources in rewriting such a substantial part of the generator. I therefore suggest that we "part ways", meaning I will put the generator in a separate repository and submit typings based on its output as |
Yes, I think that makes perfect sense. I'm very sorry that we didn't coordinate with you better from the beginning, and I hope this doesn't sour you on the project. We are grateful for your hard work and for the release of the output as |
Don't worry, it was a great learning experience nonetheless and we have used the generated typings in our projects successfully for some time now, so it definitely wasn't for nothing, |
@joeyparrish I agree with the approach of releasing the typings as soon as possible as I was already using the types from this pull request by copying them locally into a project, and the only way to make it work right is to name it/install it as if it were from |
Implements a node-based build tool to generate TypeScript type definitions based on the generated externs file and the externs defined in
externs/shaka/
.Fixes issue #1030