-
Notifications
You must be signed in to change notification settings - Fork 6k
[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
[Typescript][Fetch] added option to override default basePath #7609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok
@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.
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 ?
@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.
Great, I will add this today
There was a problem hiding this 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!
thanks 👍 should i do something further to get this merged, or will someone just pick this up sometime ?
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)
cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01)
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).
@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.
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 ?
@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) {
@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?
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.
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
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)
@TiFu thanks i understand now! :)
3f76000 to
639588f
Compare
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks fine
@wing328 is this ready to merge? I cannot retrigger the circleci build, maybe you have the permission to do that ?
639588f to
54c8990
Compare
swagger-bot
commented
Jul 1, 2020
Hey Meat Bag! Your Pull Request Build Passed!
743 tests run, 5 skipped, 0 failed.
4ba5164 to
949f0e6
Compare
Uh oh!
There was an error while loading. Please reload this page.
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif 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\.3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.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-fetchtemplate.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_PATHto 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