Add optional erasable syntax configuration to Typescript generator.#21040
Add optional erasable syntax configuration to Typescript generator.#21040brendandburns wants to merge 2 commits intoOpenAPITools:masterfrom
Conversation
|
Is there a reason not to ALWAYS generate erasable syntax, without extra configuration and complexity in the templates? |
|
Personally, I'm ok with that, but I made it optional because I wanted to minimize the potential impact to users of this generator. |
|
I think having both options is quite the onus on future implementers and maintainers. You always have to think about both cases and what's supported might change over time as well as ecmascriot evolves, etc. Given that we want to run future versions of this on native node, I personally would go with one of two options:
Thoughts? |
|
@joscha I'm fine with either option. I defer to the various typescript technical committee members opinion. Let me know the preferred solution and I will implement it in this PR. |
|
@amakhrov @macjohnny thoughts? |
|
@brendandburns I like what you done: to be optional and by default working like before |
|
There seem to be three different opinions here :) Any ideas about how we should get to consensus? I'd like to get this PR merged so that we can start using it. |
Erasable syntax is a subset, right? So if we restrict the code to be always erasable we get node native support for free without impairment for anyone using more flexible transpilation? |
|
It is a subset, however it does eliminate |
|
the changes seem syntactically small for a gain in compatibility, making this option appealing:
|
|
I'm still looking for converged advice here. Can we merge as-is since it is a no-op for most users and then we could change the default (or even remove it entirely) in future releases? |
Sorry, you're right. I still think we should make this default from now on, with a compat flag that people can switch on in projects where is causes issues (which I don't even think will happen). |
|
@joscha lmk when this is ready to be merged |
I don't think this is just up to me? I made my point, I'd enable it by default and make it possible to disable until we've established that it is fine to keep enabled for good and we can get rid of the second code path. |
|
@brendandburns sorry for the long delay and thanks for this feature! thank you all for the good arguments. I would suggest the following to move this forward:
makes sense? |
I like what @brendandburn done and LGTM as it is, to be merged! |
|
momentum of discussion has decreased. |
|
Any plans to merge this PR anytime soon? |
|
ive opened a copy of this PR with the circleCI and merge conflicts resolved at #21560 |
|
merged in #21560 |
Typescript 5.8 added a new flag that optionally removes support for non-erasable syntax. This facilitates running Typescript directly in node 22 without transpiling. This PR adds a new configuration option to the Typescript generator to generate code that complies with erasable syntax only. It is off by default.
PR checklist
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.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04) @joscha (2024/10)