[typescript] ANTLR test enhancement. Utility types now considered when generating schemas (Issue #21317)
PR checklist
- [x] Read the contribution guidelines.
- [x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
- [x] Run the following to build the project and update samples:
(For Windows users, please run the script in WSL) Commit all changed files. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example./mvnw clean package || exit ./bin/generate-samples.sh ./bin/configs/*.yaml || exit ./bin/utils/export_docs_generators.sh || exit./bin/generate-samples.sh bin/configs/java*. IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed. - [ ] File the PR against the correct branch:
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks) - [x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.
@joscha @mkusaka , please help review
refs #21317
I am not following 100% what's going on in this pull request, but it seems like there is a lexer/parser for the typescript grammar being generated in order to find types and their generics? Whilst I can appreciate the lean solution, I don't think this is a future-safe, sustainable way to do this. Whilst being non-standard, it wouldn't automatically extend to future typescript grammars without manual update.
A few thoughts: Can we use the TS parsing functionality itself - given this recently was ported to Go, could it be portable enough to be included here? Alternatively, maybe one of the bigger AST generators like acorn has a WASM implementation that we can load? Alternatively, is there a standard implementation of the lexer/parser part in this PR? Some widespread used Java package that has this functionality and is actively developed and we can lean on? Alternatively, (this is already quite sad) is there a module that can load native JS in Java (Rhino comes to mind, but that is aeons old already) that we can use to load one of the AST generators and/or ts package itself inside java? Alternatively, we can expect a dependency on node and call out to it at generation time to parse TS with its default tooling (this is probably the most future-proof but also the most overhead, unexpected solution)?
Good Day, @joscha , thank you for your feedback, These are my responses:
Can we use the TS parsing functionality itself - given this recently was ported to Go, could it be portable enough to be included here?
I'm not fully sure what you mean by "portable", but I'm also not sure what that "ported to Go" comment from the g4 file meant either, so maybe that's where the misunderstanding starts
Maybe one of the bigger AST generators like acorn has a WASM implementation that we can load?
I don't know how WASM works, so I can't really speak to this
Is there a standard implementation of the lexer/parser part in this PR? Some widespread used Java package that has this functionality and is actively developed and we can lean on?
I'm not sure if I can call it "standard", but I got the files from the Grammars repository, and the TypeScriptParser got its last update 7 months ago, on November 4 2024
Is there a module that can load native JS in Java (Rhino comes to mind, but that is aeons old already) that we can use to load one of the AST generators and/or ts package itself inside java?
I don't know much about Rhino, but from what I've looked up, I don't see anything about it supporting TypeScript
We can expect a dependency on node and call out to it at generation time to parse TS with its default tooling
I've thought about that, and then it also came to my mind that if we were to do similar for every language that this project supports, then it would be quite the amount of overhead
Overall, I get where you're coming from. The fact that I couldn't even write all my unit tests within this PR without running into a parsing bug proves that parsing - as it is currently may - not be that much of an enhancement. I could try rolling back these assertion classes and use regular String comparisons instead. Let me know if that's okay
Just a short note that I don't think the intent if this PR is bad, quite the opposite, I am just worried about the maintenance baggage that might come with it, especially because going forward we expect people making updates to the generator also to possibly deal with antlr, which can't be expected. All I am saying is that I wish we could reuse the actual typescript parsing logic or a package which is less low level than the grammar for the language we're trying to support. Given this is in tests and we're talking TS, maybe we can safely assume node and ecosystem to be there? I know that the CLI package also has a nix profile, which alleviates a bunch of issues with dependency management - maybe that's an option here, too? @macjohnny @wing328 WDYT?
@macjohnny , thank you for the review. Please do you think this properly addresses the original issue? @joscha , I've removed ANTLR by the advice of macjohnny. Do you think this fixes the main issue?
@joscha , I've removed ANTLR by the advice of macjohnny. Do you think this fixes the main issue?
yes, i think so :)
Okay. Thank you both. I'll revert by the weekend or sooner
@macjohnny @joscha , I've made the changes, but it seems I have to wait for some issues to be fixed from upstream as relates to meta-codegen
@DavidGrath please update the docs and the samples and merge the latest master branch.
@macjohnny Done
@DavidGrath can you please merge the most recent master branch and re-generate the samples and docs? looks like 5000+ files were changed because of some version string
@macjohnny , please can you confirm? I should have already merged the 7.14.0 release with 600ad25 before I generated the docs
@DavidGrath yeah lets just try to merge the latest master, and re-generate the samples and docs, that should hopefully get rid of the unrelated version string changes
@macjohnny , okay, I'll try and revert between now and Friday
@macjohnny , sorry for the delay, I've merged and regenerated the 7.15-snapshot samples