openapi-generator icon indicating copy to clipboard operation
openapi-generator copied to clipboard

[typescript] ANTLR test enhancement. Utility types now considered when generating schemas (Issue #21317)

Open DavidGrath opened this issue 7 months ago • 5 comments

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:
    ./mvnw clean package || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (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 ./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 (upcoming 7.x.0 minor 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

DavidGrath avatar Jun 14 '25 12:06 DavidGrath

refs #21317

joscha avatar Jun 14 '25 20:06 joscha

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)?

joscha avatar Jun 14 '25 21:06 joscha

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

DavidGrath avatar Jun 15 '25 17:06 DavidGrath

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?

joscha avatar Jun 15 '25 19:06 joscha

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

DavidGrath avatar Jun 18 '25 09:06 DavidGrath

@joscha , I've removed ANTLR by the advice of macjohnny. Do you think this fixes the main issue?

yes, i think so :)

joscha avatar Jun 18 '25 14:06 joscha

Okay. Thank you both. I'll revert by the weekend or sooner

DavidGrath avatar Jun 19 '25 08:06 DavidGrath

@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 avatar Jun 21 '25 18:06 DavidGrath

@DavidGrath please update the docs and the samples and merge the latest master branch.

macjohnny avatar Jun 23 '25 07:06 macjohnny

@macjohnny Done

DavidGrath avatar Jun 25 '25 04:06 DavidGrath

@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 avatar Jun 25 '25 08:06 macjohnny

@macjohnny , please can you confirm? I should have already merged the 7.14.0 release with 600ad25 before I generated the docs

DavidGrath avatar Jun 25 '25 09:06 DavidGrath

@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 avatar Jun 25 '25 09:06 macjohnny

@macjohnny , okay, I'll try and revert between now and Friday

DavidGrath avatar Jun 25 '25 10:06 DavidGrath

@macjohnny , sorry for the delay, I've merged and regenerated the 7.15-snapshot samples

DavidGrath avatar Jun 29 '25 10:06 DavidGrath