[typescript][angular] basePath through injected config#289
Conversation
|
@akehir thanks a lot for your PR! |
macjohnny
left a comment
macjohnny
left a comment
There was a problem hiding this comment.
I think the idea of using this.configuration.basePath looks good. I suggest a small change in order to account for a situation with a configuration without base path and a separate base path.
| @@ -67,6 +67,8 @@ export class {{classname}} { | |||
| if (configuration) { | |||
There was a problem hiding this comment.
wouldn't you want to set
if (configuration) {
this.configuration = configuration;
this.basePath = basePath || configuration.basePath || this.basePath;
}
this.configuration.basePath = this.basePath;
in order to account for the situation where you pass a configuration object that does not contain a base path plus a basePath parameter, since the basePath property of Configuration is optional, see
https://github.com/openapitools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/typescript-angular/configuration.mustache#L15
?
There was a problem hiding this comment.
I would even mark the basePath property of the api service as deprecated, since only this.configuration.basePath should be used. what do you think about that?
|
cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) |
|
@akehir which tests are missing? I think we should add them again if they went missing... |
* Expanding customization docs As a new user I was very confused on how to either modify, override or create a new library. The existing docs were brief and had requirred knowledge that new users will not have, and it took a lot of trial and error to make progress. Also the wording seemed off. It kept talking about "client" when this project creates more than just clients, used "language" when some templates are not a language, and used library to mean a few seeminly different things. I've tried to consolidate the wording around "template" and "codegen". The codegen is the Java code, and then you give it a "name" which is used for the template. That's the template folder it will live in, so it seems to work. Feel free to use this as a base and edit it out, but this will all make a lot more sense to beginners. :)
Updates some confusing wording, specifies naming conventions, and links to external `new.sh` for contributed templates (rather than modified templates).
…ed. Updated code to handle the case that configuration.basePath is not set
|
@macjohnny I've updated the code to handle the case that configuration.basePath is not setup. I'm now also using the configuration.basePath everywhere (was not used in the code generated for the old httpclient). As for the tests, they might have been removed intentionally (especially, the angular 5 test, which was previously there). However, the scripts for generating the angular 6 tests were missing from the unix .bin/ and only existed in the windows .bat - which was the main reason for my confusion. I've created the unix scripts for generating the angular 6 test cases, and updated those with my newly generated codegen. |
|
@macjohnny Although, the angular6 tests are not tested by the CI as far as I can see (there's no pom.xml / the tests & all the angular modules / builds except 4 & 4.3 are commented out). Therefore, maybe the typescript test cases would need to be overhauled in general =/ |
| @@ -67,6 +67,8 @@ export class {{classname}} { | |||
| if (configuration) { | |||
…nAPITools#274) * Add possibility to build and run go service in a container * Remove tabs * Update Pet Store server sample * Add {{{packageName}}} instead of petstore
* First attempt at getting rust-server working * Solve the problem of spurious 'object's * We've found the missing models * Catch some single-var objects correctly * Get single-param models 'working' * Got files working * Remove surplus logging * Disable some things to get it compiling * `cargo test` now passes as well * Create rust-server-specific petstore.yaml We've commented out a few bits that rust-server doesn't yet support * Remove commented-out code And finally get rid of the generation date in the sample
* Add rust-server to travis CI * Install rust correctly (and only once)
@akehir @wing328 I think we should consider running those tests in CI, what do you think? |
|
@macjohnny totally agreed running more tests to have better coverage on the auto-generated code. |
|
@akehir would you be willing to give it a try and include your test case in the CI configuration? |
* fix cast problem * fix cast problem wing328 suggestion
…ITools#298) * Configuration option to disable HTML escaping when using Gson The default implementation of Gson will escape certain characters by default. This includes the `=` character, which is used in base64 encoding and cause problems when deserializing the value to a base64 encoded string in a service. Adding an option for disabling this feature makes it easier to generate client code with sane defaults. * Update Petstore sample
|
A good starting point is https://github.com/OpenAPITools/openapi-generator/blob/master/pom.xml#L921 You will need to add a pom.xml similar to https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/typescript-angular-v4.3/npm/pom.xml |
|
You may also reference the Petstore integration tests for TS Fetch client: https://github.com/OpenAPITools/openapi-generator/tree/master/samples/client/petstore/typescript-fetch/tests/default (Typescript Angular petstore samples do not have integration tests at the moment and we would welcome contribution to add those) Let us know if you need any help. Thanks again for your contribution. |
|
@akehir here is an example of CircleCI with NodeJS / Chrome image: it works perfectly to run tests within the image with chrome launcher: https://github.com/bithost-gmbh/ngx-mat-select-search/blob/master/karma.conf.js#L10 The test-result: Or am I missing something that is different with the OpenAPI Generator setup? |
|
@macjohnny : Your example is quite different - mainly, your complete tests run within docker. In the circleci here, a docker image is first built on the ci vm, and then started. Since it's a VM, Chrome would need to be added via apt-get. I see, that @wing328 has now migrated to circleci 2.0 scheme for the building, but my branch is still behind that, so changing the ci config would first require rebasing 💩 |
…ed. Updated code to handle the case that configuration.basePath is not set
…rm with typescript-fetch sample directory structure
…injected configuration
…o have the same structure
…ehir/openapi-generator into feature/basePathThroughInjectedConfig
akehir
changed the title
|
@macjohnny : I've tried to enable the tests with chrome headless for circleci and travis. Let's see what happens ;-) |
|
@macjohnny : CircleCI seems to work now with the test (on the machine itself). |
|
Awesome! |
|
@akehir could you please close and re-open your PR to re-trigger the circleCI build? |
|
@chrismellard could you test this branch to see if it also resolves your issue swagger-api/swagger-codegen#7949 ? |
|
Looks like a rebase (on master) has not been done correctly resulting in a lot of commits incorrectly added to this PR/branch. I would suggest opening a new PR instead (by cherry-picking the commits) |
|
the circleCI build fails again: |
16 participants
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\.master,3.1.x,4.0.x. Default:master.Description of the PR
@macjohnny : This resolves #282 , implementing my swagger-codegen pull request on the fork.
I notice, that there are a few tests missing, notably the ones I've previously added for angular 6.
CI is here: https://travis-ci.org/akehir/openapi-generator
The original PR here: swagger-api/swagger-codegen#8305
Original description of the PR
Instead of using the this.basepath, we use this.configuration.basepath - allowing us to inject a different configuration (after loading a configuration file over http during APP_INITIALIZER).
We've been using this setup at a project for a while and it has been working well for us (different api url's in different environments, provided by the webserver on runtime).
This would close the problems discussed here: #3221 (comment) and #7949.
This the distinction to the existing PR #7965 is, that it is not required to generate / handle an observable, but rather just by changing the injected configuration, another basePath would be used. Do you think this would solve your concerns @chrismellard?
Since the rest of the swagger api client is always referring to the this.configuration.{...}, I believe this approach might be more consistent with the general handling of the configuration service.
To keep compatibility with implementations that don't inject a configuration, but just the base path, I set the base path on the internal configuration object to the injected basepath (when they don't exist).