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-rxjs] runtime and api cleanup #3316

Merged
merged 13 commits into from
Jul 22, 2019

Conversation

denyo
Copy link
Contributor

@denyo denyo commented Jul 9, 2019

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
  • Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This PR simplifies models RequestArgs and ResponseArgs and makes them an alias for { AjaxRequest, AjaxResponse } from 'rxjs/ajax';. Also parameter names make more sense now and unneeded checks are removed.
For people that have previously applied any middlewares this is breaking due to the model changes but it is quite easy to fix. Docs where also extended.

Middleware Before

export class AuthInterceptor extends Configuration {
  private static config: AuthInterceptor;

  private constructor() {
    const middleware: Middleware[] = [
      {
        pre: (context: RequestContext): RequestArgs => ({
          ...context,
          options: {
            ...context.options,
            headers: {
              ...context.options.headers,
              Authorization: `Bearer ${getAuthToken()}`
            }
          }
        })
      }
    ];

    super({ middleware });
  }

  public static get Instance() {
    return AuthInterceptor.config || (AuthInterceptor.config = new this());
  }
}

Middleware After

export class AuthInterceptor extends Configuration {
  private static config: AuthInterceptor;

  private constructor() {
    const middleware: Middleware[] = [
      {
        pre: (request: RequestArgs): RequestArgs => ({
          ...request,
          headers: {
            ...request.headers,
            Authorization: `Bearer ${getAuthToken()}`,
          },
        }),
      },
    ];

    super({ middleware });
  }

  public static get Instance() {
    return AuthInterceptor.config || (AuthInterceptor.config = new this());
  }
}

@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @nicokoenig (2018/09) @topce (2018/10)

@auto-labeler
Copy link

auto-labeler bot commented Jul 9, 2019

👍 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
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

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

the changes are not backwards-compatible. so this should go to the 5.x branch or we should prefer the current interfaces. what do you think?

@@ -184,12 +171,11 @@ export const throwIfRequired = (params: {[key: string]: any}, key: string, nickn
}
}

export interface RequestContext extends RequestArgs {}
export interface ResponseContext extends RequestArgs {
Copy link
Member

@macjohnny macjohnny Jul 9, 2019

Choose a reason for hiding this comment

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

this isnt backwards compatible, so I would not delete ResponseContext

map((response) => {
if (postMiddlewares) {
postMiddlewares.forEach((mw) => (response = mw.post!({ ...params, response })));
Copy link
Member

@macjohnny macjohnny Jul 9, 2019

Choose a reason for hiding this comment

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

this isn't backwards-compatible so I would revert it

@macjohnny
Copy link
Member

cc @akehir

@macjohnny
Copy link
Member

according to https://github.com/OpenAPITools/openapi-generator/wiki/Git-Branches it should be ok to merge this into 4.1.x, as the breaking-change is easy to fix

@macjohnny macjohnny changed the base branch from master to 4.1.x July 9, 2019 08:35
@macjohnny macjohnny changed the base branch from 4.1.x to master July 9, 2019 08:35
@macjohnny
Copy link
Member

macjohnny commented Jul 9, 2019

@denyo can you cherry-pick into 4.1.x?
4.0.3 is the last patch release before 4.1.0, so it is fine to merge this into master

@akehir
Copy link
Contributor

akehir commented Jul 9, 2019

@denyo can you cherry-pick into 4.1.x?
4.0.3 is the last patch release before 4.1.0, so it is fine to merge this into master

@macjohnny, during the release, the 4.1.0 branch would be merged into master, and future 4.1.x patch releases would be branched of master (unexpectedly containing the breaking change). Merging to master seems to violate the merge direction description of the git branches wiki entry.

How are breaking changes documented in order to help people experiencing the breakage to be able to fix this easily?

I understand that simplifying the template is a good idea, however, I would be reluctant to introduce unexpected breaking changes (especially since whoever doesn't like the template can easily override it in the local project), if it cannot be communicated properly.

@denyo
Copy link
Contributor Author

denyo commented Jul 9, 2019

I kind of had the same thoughts as @akehir.
On the one hand I am all for simplification and performance, on the other I am not a fan of breaking changes either in patch or minor version bumps.
In general until recently with 1 or 2 bug reports about this particular generator I had the feeling it wasn't really used by anyone so I wasn't too worried. Are there any usage stats about this?
Since it is initially based on typescript-fetch some of their technical debt was obvisouly dragged over. So this PR is mainly towards cleaning this up since rxjs is providing the ideal interfaces already.

@macjohnny
Copy link
Member

now that 4.0.3 has been released, the next release (i.e. 4.1.0) will be taken from master, as the 4.1.x branch should be merged into master now.
in the next minor release (4.1.0), breaking changes with fallback are permitted. I labelled this PR with Breaking changes with fallback, so it can be found in the release notes (see e.g. https://github.com/OpenAPITools/openapi-generator/releases/tag/v4.0.0), so the migration steps should be documented here in this PR. @denyo can you do that?

@denyo
Copy link
Contributor Author

denyo commented Jul 10, 2019

@macjohnny migration steps are in the description with before and after? Or what do you mean?

@macjohnny
Copy link
Member

migration steps are in the description with before and after? Or what do you mean?

I think the description in the PR should be enough, but maybe it might be worth adding an additional line explaining explicitly which signature changed.

@wing328 wing328 added this to the 4.1.0 milestone Jul 12, 2019
@denyo denyo changed the title [typescript-rxjs] runtime cleanup [typescript-rxjs] runtime and api cleanup Jul 13, 2019
@macjohnny macjohnny merged commit d2576f4 into OpenAPITools:master Jul 22, 2019
@denyo denyo deleted the feature/rxjs-runtime-cleanup branch July 22, 2019 09:30
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.

5 participants