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

[typescript-angular] drop support of angular below 6.0.0 #6360

Merged
merged 3 commits into from
May 28, 2020

Conversation

macjohnny
Copy link
Member

Remove support for old angular versions below 6.0.0, allowing to clean up the code base.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

cc @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)

@auto-labeler
Copy link

auto-labeler bot commented May 19, 2020

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

Copy link
Contributor

@amakhrov amakhrov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job @macjohnny !
All looks good to me.

I only have one question re string enums.

Should we preserve some way to emit enums in namespaces?
With real string enums it's the only way to make two structurally identical enums inter-assignable. Which can be a handy workaround for OAS2 limitation (which doesn't allow to re-use a enum and requires to redefine enum for every field).

Example:

Query1:
  properties:
  - sort:
    type: string
    enum: [asc, desc]

Query2:
  properties:
  - sort:
    type: string
    enum: [asc, desc]

It emits two enums:

enum Query1SortEnum {
  asc='asc',
  desc='desc'
}

enum Query2SortEnum {
  asc='asc',
  desc='desc'
}

In TS, enums are not assignable to each other even if structurally compatible. So I can't do this:

let q1!: Query1;
let q2: Query2 = q1;

However, namespaces solve that. If the same enum (with the same name!) is defined in two different namespaces, TS allows assigning one to another. That's what we do in our project currently (with tweaked mustache templates)

namespace Query1 {
  export enum Sort {
    asc='asc',
    desc='desc'
  }
}
namespace Query2 {
  export enum Sort {
    asc='asc',
    desc='desc'
  }
}

Previosuly, with stringEnums=false (default) it was not an issue at all, because they were not real enums.
Now we always emit enums, which can introduce this TS issue with OAS2-compliant specs.

Any thoughts?

@TiFu
Copy link
Contributor

TiFu commented May 21, 2020

This raises a philosophical question: Do we want to fix the shortcomings of OAS2 with an implementation that tries to be smart?

In the case that you described, what happens if a new enum value is introduced only for Query2SortEnum (let's say random)?
Does TS still allow what you are trying to do or would this potentially cause errors totally unrelated to the actual change in the spec?

@amakhrov
Copy link
Contributor

Does TS still allow what you are trying to do or would this potentially cause errors totally unrelated to the actual change in the spec?

You will be able to assign Query1SortEnum values to variables of type Query2SortEnum but not vice versa.

Do we want to fix the shortcomings of OAS2

I'm not a big proponent of that approach. I mostly wanted to point out that the change in the PR can have backward-incompatible outcome without a fallback even for angular 8/9 users.

On the other hand, e.g. axios generator has never had such stringEnum option - it just always emits enums. We can stick to this approach for consistency across typescript generators.

@wing328
Copy link
Member

wing328 commented May 22, 2020

If it's a limitation only with OAS2, then I would suggest asking users to migrate to OAS3 spec instead.

@amakhrov amakhrov mentioned this pull request May 27, 2020
5 tasks
@macjohnny macjohnny force-pushed the feature/typescript-angular-cleanup branch from ea96716 to ff1c72d Compare May 28, 2020 06:40
@macjohnny
Copy link
Member Author

In order to minimize the breaking changes, I decided to revert the changes related to the string enums, since the functionality is already implemented and not too much of a burden.

@macjohnny macjohnny merged commit ec39005 into master May 28, 2020
@macjohnny macjohnny deleted the feature/typescript-angular-cleanup branch May 28, 2020 09:28
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request May 30, 2020
* master:
  [PS] Refactor the http signing auth with ecdsa support (OpenAPITools#6397)
  [Rust Server] Hyper 0.13 + Async/Await support (OpenAPITools#6244)
  [Rust] set supportAsync to true as the default (OpenAPITools#6480)
  [php-symfony] Set required PHP version ^7.1.3 (OpenAPITools#6181)
  update doc
  [csharp] Rename netstandard to netstandard1.3 (OpenAPITools#6460)
  feat: support deprecated parameters for typescript-axios generator (OpenAPITools#6475)
  [REQ][typescript-axios] useSingleRequestParameter should mark parameter optional if all properties are optional (OpenAPITools#6477)
  better struct alias in rust (OpenAPITools#6470)
  Migrate Go server samples to OAS 3 only (OpenAPITools#6471)
  [Rust][reqwest] add async support (OpenAPITools#6464)
  [codegen][python-experimental] Composed schema with additionalProperties  (OpenAPITools#6290)
  [Java] Decommission Retrofit 1.x support (OpenAPITools#6447)
  remove scala oas3 samples (OpenAPITools#6446)
  [Java][jersey2] Fix RuntimeException when HTTP signature parameters are not configured (OpenAPITools#6457)
  [Java][jersey2] Improve http signature code comments (OpenAPITools#6463)
  [typescript-angular] drop support of angular below 6.0.0 (OpenAPITools#6360)
  [cli] new 'author template' command (OpenAPITools#6441)
  python-experimental updates ancestor + adds descendant discriminator tests (OpenAPITools#6417)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants