Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Typescript][Fetch] added option to override default basePath #7609

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

Open
zwenza wants to merge 3 commits into swagger-api:master
base: master
Choose a base branch
Loading
from zwenza:zwenza/issue-5494_typescript-fetch_global_basePath_configuration

Conversation

@zwenza
Copy link

@zwenza zwenza commented Feb 7, 2018
edited
Loading

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 and ./bin/security/{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\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

I joined the discussion in Issue #5494 because i would need the possibility to configure a custom default basePath for all generated API's with the typescript-fetch template.

Currently afaik you can only update the base-path for a specific api via the configuration object in the api constructors. But instead of writing this in every API creation i would like to specify a default basePath used by all APIs.

This PR would add a function to the API to set the default base-path BASE_PATH to another value.

I am very open to other ideas how we could tackle this problem. This would be my first working suggestion.

(details of the change, additional tests that have been done, reference to the issue for tracking, etc)

cc @macjohnny

twosigmajab reacted with thumbs up emoji
Copy link
Contributor

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

looks ok

Copy link
Contributor

@zwenza this requires setting the base path for every api, which is ok for now, but we could improve it to use a single configuration class like https://github.com/swagger-api/swagger-codegen/blob/v2.3.0/modules/swagger-codegen/src/main/resources/typescript-angular/configuration.mustache#L10 with static parameters that can be changed.

Would you like to give it a try? You would also need to include that file in the generation similar to https://github.com/swagger-api/swagger-codegen/blob/v2.3.0/modules/swagger-codegen/src/main/java/io/swagger/codegen/languages/TypeScriptAngularClientCodegen.java#L92 in order to have the configuration.mustache processed once.

Copy link
Author

zwenza commented Feb 8, 2018

Sure I can do that!
Just to check if i understand you correct: you want to add a additional moustache for that global configs, not in the existing configuration.moustache ?

capsur reacted with thumbs up emoji

Copy link
Contributor

@zwenza you are right, we could add a GlobalConfiguration class containing static variables only in the existing https://github.com/swagger-api/swagger-codegen/blob/v2.3.0/modules/swagger-codegen/src/main/resources/typescript-fetch/configuration.mustache and since it is already included in the api files, we simply need to use the base path from that static variables instead of the constant defined in the template.

Copy link
Author

zwenza commented Feb 8, 2018

Great, I will add this today

Copy link
Contributor

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

looks good to me.
thanks for your contribution!

Copy link
Author

zwenza commented Feb 9, 2018

thanks 👍 should i do something further to get this merged, or will someone just pick this up sometime ?

Copy link
Contributor

@zwenza the PR should be ready to be merged. @wing328 should eventually merge it if no issues arise.

zwenza reacted with thumbs up emoji

Copy link
Contributor

wing328 commented Feb 10, 2018

Is this similar to the singleton approach to have global properties (e.g. basePath)?

We started with the singleton approach for some clients (e.g. Python) and moved away due to various issues (e.g. multi-threading)

Copy link
Contributor

wing328 commented Feb 10, 2018

cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01)

Copy link
Contributor

TiFu commented Feb 10, 2018

Looks like it is @Wing32 .

I agree with your concerns. I'd prefer an object based approach and just using a 'default' object for the instantiation of the api. This is far more flexible and doesn't run into any multi threading issues (Webworkers are a thing).

Copy link
Contributor

@TiFu @wing238. I agree. Object based approach would be best .

I'd even love to see this in the base typescript but I'd need to go back and see how it would flow.

Copy link
Author

zwenza commented Feb 10, 2018

I agree that the current solution could lead into such problems.
If i understand you right, you suggest to provide a configurable default object which i then would pass each manually at instantiation ?

Copy link
Contributor

TiFu commented Feb 10, 2018

@zwenza yeah. The default configuration object should be passed by default, maybe similar to portableFetch and GlobalConfiguration.basePath in this sample:

constructor(configuration?: Configuration, protected basePath: string = GlobalConfiguration.basePath, protected fetch: FetchAPI = portableFetch) {

Copy link
Author

zwenza commented Feb 15, 2018

@TiFu so you mean instead of passing only the basePath (which is the current state in my solution) you would pass the whole configuration object?

How would this fix the multi threading issues?

Copy link
Contributor

TiFu commented Feb 15, 2018
edited
Loading

Maybe I wasn't clear enough what my idea was.

The configuration object should no longer be static, so that you can create multiple objects (e.g. one for each thread), so that they can be changed independently. This solves the multi threading issue.

The second idea was to pass the configuration object directly, so that someone can easily add more options e.g. auth settings to it and use them in the api. Anyway, I just noticed that there's a Configuration object already, which contains the basepath?
This object is used in L. 49 of api.mustache .

Why do we not provide a default object for configuration (using configuration: Configuration = defaultObject) in the constructor in api.mustache?
The constructor of Configuration could set appropriate default values (e.g. the base path contained in GlobalConfiguration l. 5. A value passed by the parameter param of Configuration#constructor should take precedence over the default values.

This would allow changing all available settings, without passing all the parameters manually. Sorry, I have no idea why I didn't notice that earlier.

Copy link
Author

zwenza commented Feb 15, 2018
edited
Loading

If i make the basePath non-static wouldn't i have the problem again that i cannot set the basePath global to another value?

The main issue i want to resolve with this PR is that you have the ability to change the default basePath for all generated APIs

Copy link
Contributor

TiFu commented Feb 15, 2018
edited
Loading

If you only ever use the same object instance of GlobalConfiguration (or Configuration e.g. a default instance which could be exported by configuration.mustache) that wouldn't be a problem.

Let me make the idea a bit clearer with a gist: https://gist.github.com/TiFu/e5efe5a920dfd1c54231d98cc9bfc8d8

Would this solve your issue or am I missing something obvious? If you have any concerns about this approach let me know.

Maybe someone else from the technical committee can also take a look and give some feedback.
@taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01)

Copy link
Author

zwenza commented Feb 15, 2018

@TiFu thanks i understand now! :)

@zwenza zwenza force-pushed the zwenza/issue-5494_typescript-fetch_global_basePath_configuration branch 5 times, most recently from 3f76000 to 639588f Compare April 4, 2018 05:21
Copy link
Author

zwenza commented Apr 4, 2018
edited
Loading

@TiFu @macjohnny i implemented the proposed solution. Can you review the changes again?

It seems like the circleci build failed because it wasn't able to fetch a dependency but i cannot retrigger the build.

Copy link
Contributor

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

looks fine

Copy link
Author

zwenza commented Apr 5, 2018

@wing328 is this ready to merge? I cannot retrigger the circleci build, maybe you have the permission to do that ?

capsur and wedi reacted with thumbs up emoji

@zwenza zwenza force-pushed the zwenza/issue-5494_typescript-fetch_global_basePath_configuration branch from 639588f to 54c8990 Compare April 6, 2018 13:12
Copy link

Hey Meat Bag! Your Pull Request Build Passed!
743 tests run, 5 skipped, 0 failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@macjohnny macjohnny macjohnny approved these changes

@helpmefindaname helpmefindaname helpmefindaname approved these changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

v2.4.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /