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

JS client has hard-coded service URL #2026

Closed
delenius opened this issue Feb 3, 2016 · 14 comments
Closed

JS client has hard-coded service URL #2026

delenius opened this issue Feb 3, 2016 · 14 comments

Comments

@delenius
Copy link
Contributor

delenius commented Feb 3, 2016

My swagger spec has something like

basePath: "/foo/bar"

Based on this, the JS client generator produces this code in ApiClient.js

this.basePath = 'http://localhost/foo/bar'.replace(/\/+$/, '');

It would be much nicer if the service URL was not hard-coded into the generated client code, especially since it happens to be wrong in this case: My service also needs a port number. I don't think the server URL is normally part of the swagger spec. And for good reason: During the lifecycle of a service, it may run in different places (for development/testing/production, etc).

Note that swagger-js allows you to pass the URL as a parameter:

var Swagger = require('swagger-client');

var client = new Swagger({
  url: 'http://petstore.swagger.io/v2/swagger.json',
  success: function() {
    client.pet.getPetById({petId:7},{responseContentType: 'application/json'},function(pet){
      console.log('pet', pet);
    });
  }
});
@delenius
Copy link
Contributor Author

delenius commented Feb 3, 2016

Oh, I see now that the spec allows for a separate "host" field. Still, it would be nice to be able to pass this at runtime. Currently, since I am generating my swagger spec from the Java server code, the Java source code has to know where the service will be deployed. Not good separation of concerns.

@delenius
Copy link
Contributor Author

delenius commented Feb 3, 2016

To be more precise, the ApiClient constructor could take the host as an optional argument, which would then override the default.

@fehguy
Copy link
Contributor

fehguy commented Feb 3, 2016

I'm not trying to complicate things, but there are some other options with javascript. Have you seen the swagger-js project? All these things are covered there.

@delenius
Copy link
Contributor Author

delenius commented Feb 3, 2016

Yes, that's why I pointed to that project above :)

But I much prefer to generate the client statically. It has many advantages:

  • Much less overhead at runtime (time to generate client, network transfer for large codegen library and large swagger file, etc).
  • The statically generated client can be minified and uglified.
  • The statically generated client can be used to produce client documentation (with JSDoc or the like).

It just doesn't make much sense to me to generate the client at runtime.

@fehguy
Copy link
Contributor

fehguy commented Feb 3, 2016

no problem at all :) and now I see that you obviously knew about it

@xhh
Copy link
Contributor

xhh commented Feb 4, 2016

The basePath field of ApiClient has a default value and could be overridden. For example:

ApiClient.default.basePath = 'http://example.com/v2';
var petApi = new PetApi(); // using ApiClient.default if no argument is provided

var devClient = new ApiClient();
devClient.basePath = 'http://localhost:8080';
var devPetApi = new PetApi(devClient);

@delenius
Copy link
Contributor Author

delenius commented Feb 4, 2016

@xhh, ok, but that still combines the host and basePath into one. In swagger, these are two different properties, "host" and "basePath".

I'd like to only change the host part, and be able to use the basePath from the swagger spec.

@wing328
Copy link
Contributor

wing328 commented Feb 4, 2016

@delenius we prefer combining host and basePath into one as API clients in other languages behave the same and we're trying deliver a consistent developer experience across different langauges.

@wing328 wing328 added this to the v2.1.6 milestone Feb 4, 2016
@delenius
Copy link
Contributor Author

delenius commented Feb 4, 2016

@wing328 ok, then I will use the method above to set the whole thing.

@wing328
Copy link
Contributor

wing328 commented Feb 4, 2016

@delenius thanks. Please let us know if you've further feedback. Btw, we just merged #2030 and you may want to pull the latest master to generate the Javascript API client again.

@wing328 wing328 closed this as completed Feb 4, 2016
@delenius
Copy link
Contributor Author

delenius commented Feb 4, 2016

Will do, thanks! 👍

@yongdange
Copy link

@delenius, Any progress on this issue?

@delenius
Copy link
Contributor Author

@hc-romens, this was closed a long time ago. See above for a workaround (which I use).

@yongdange
Copy link

@delenius somehow, swagger-js is not the solution I want. Cause, the endpoint APIs defined in swagger.json are allowed to be published and public.

I am reading the source code, and try to make a patch for the statis generated js client code to take the host as the module param. So that the host value can be passed in as the command line option.

Anyway, thanks a lot!

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

No branches or pull requests

5 participants