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

[zwenza]

PR checklist

• [x] Read the contribution guidelines.
• [x] 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\.
• [x] Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
• [x] 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

[macjohnny]

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

[zwenza]

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 ?

[macjohnny]

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

[zwenza]

Great, I will add this today

[zwenza]

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

[macjohnny]

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

[wing328]

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)

[wing328]

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

[TiFu]

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

[kenisteward]

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

[zwenza]

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 ?

[TiFu]

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

[zwenza]

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

[TiFu]

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.

[zwenza]

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

[TiFu]

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)

[zwenza]

@TiFu thanks i understand now! :)

[zwenza]

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

[zwenza]

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

[swagger-bot]

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