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 type definitions #1273

Conversation

niklaskorz
Copy link
Contributor

@niklaskorz niklaskorz commented Feb 3, 2018

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

…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
@niklaskorz
Copy link
Contributor Author

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

TheModMaker pushed a commit that referenced this pull request Dec 18, 2019
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
@niklaskorz
Copy link
Contributor Author

I've updated the branch to shaka-player v2.5.8 and published an according build at @alugha/shaka-player for anyone wanting to try the typed package.

Copy link
Member

@joeyparrish joeyparrish left a 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.

.eslintrc.js Outdated Show resolved Hide resolved
build/compiler.py Outdated Show resolved Hide resolved
build/compiler.py Outdated Show resolved Hide resolved
externs/shaka/ads.js Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
externs/ima.js Outdated Show resolved Hide resolved
externs/shaka/player.js Outdated Show resolved Hide resolved
@niklaskorz
Copy link
Contributor Author

@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.

@niklaskorz
Copy link
Contributor Author

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

@niklaskorz
Copy link
Contributor Author

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.

@CHaNGeTe
Copy link

What is stopping this PR to be merged?

@niklaskorz
Copy link
Contributor Author

niklaskorz commented Jun 11, 2020

@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.

@joeyparrish
Copy link
Member

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 build/generateExterns.js. I think it makes more sense to add typescript output to that instead of repeating the effort of parsing the code in the first place.

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.

@niklaskorz
Copy link
Contributor Author

@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 @types/shaka-player outside of the shaka-player repository, which is the usual way for publishing community maintained TypeScript typings of a library. You are free to reuse any code from the pull request though. :) If you ever get to releasing official TypeScript typings as part of the npm shaka-player package, the @types/shaka-player package can be updated to return a warning to use the official typings instead.

@joeyparrish
Copy link
Member

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 @types/shaka-player. It will be a clear benefit to the community. Please let us know in #1030 when you've released those, and we will coordinate with you on how to make the transition when we have a solution that we're ready to integrate into this repo.

@niklaskorz
Copy link
Contributor Author

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,

@ngbrown
Copy link

ngbrown commented Jul 22, 2020

@joeyparrish I agree with the approach of releasing the typings as soon as possible as @types/shaka-player. This will benefit users of existing users of shaka-player instead of having to wait for the next release.

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 @types/shaka-player.

@joeyparrish joeyparrish closed this Oct 8, 2020
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status: archived Archived and locked; will not be updated type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants