Skip to content

Conversation

@pixelshaded
Copy link
Contributor

The request node module normally sends arrays in the querystring like so

?array[0]=1&array[1]=2

However Web API doesn't know how to bind that to the model. Web API supports this method of serialization:

?array=1&array=2

The request module allows for you to override this default functionality so you can produce the second outcome. This pull request adds support for setting useQuerystring on the instance of the api so that all future calls can use the second method of serialization.

jaz-ah and others added 29 commits April 6, 2016 15:20
… going to match the enum name - now that we camelCase variable names this cuts down on the amount of enum escaping we have.
…into travis

# Conflicts:
#	samples/client/petstore/go/go-petstore/.travis.yml
Adds a .swagger-codegen-ignore file with instructions and examples.
The .swagger-codegen-ignore file is treated as a supporting file.

Every project will generate a .swagger-codegen-ignore file containing
instructions and examples.

This also adds support for 'common' files (defaults like
.swagger-codegen-ignore). In the case of the ignore file, a generator
may include a compiled template ignore file which outputs to the
outputDir folder as .swagger-codegen-ignore and the default file
generation will honor the already generated file.

The rules for .swagger-codegen-ignore are a simple subset of what you'd
find in .gitignore or .dockerignore. It supports recursive matching
(**), simple matching (*), matching files in the project root
(/filename), matching against directories (dir/), negation rules
(!previously/excluded/**/file).
…ues speaking to the petstore service - has to do w/ Accept/content type changes
@pixelshaded pixelshaded changed the title Querystring Add Support for useQuerystring Request Option May 18, 2016
@wing328 wing328 added this to the v2.2.0 milestone May 24, 2016
@pixelshaded
Copy link
Contributor Author

pixelshaded commented May 24, 2016

We are now versioning the output javascript files (client.js, api.js, etc)? Shouldn't these be ignored and the ts files be the source of truth? Will update things how I think they should be and then await feedback.

Alexander Fisher added 4 commits May 24, 2016 16:10
@pixelshaded
Copy link
Contributor Author

Looks like the rebase blew up this pr.

@wing328
Copy link
Contributor

wing328 commented May 25, 2016

@pixelshaded no worry. I'll cherry-pick your commits later today.

@Vrolijkx
Copy link
Contributor

@pixelshaded If we only version the .ts files the bundeling will become the burdon of the consumer. Also it will become very hard for non typescript angular2 users that like ES5 to use these clients. Maybe we should version both the .ts and the .js files.

@pixelshaded
Copy link
Contributor Author

@Vrolijkx

  1. Why is the consumer concerned with bundling server side scripts? And if they are, why should codegen be handling that? Bundling is almost always tightly coupled to the consumer's build system. Why even try to account for something that is not even standardized?
  2. How would angular2 use a node client? I wouldn't expect support for running a node library in the browser, even if you can make it happen with things like browserify. I would expect the consumer to be responsible for this overhead, not codegen. Right now, browserify and request (which we use in typescript-node) don't play nicely for multiple reasons: Does "request" have browser support? request/request#2090. You want to validate that every dependency we use can be run client side? In which module loaders?
  3. The js files in the samples folder aren't generated from codegen. You have to run the ts files through a compiler manually before committing to even update them (just like a consumer would). It really doesn't make sense to version them. They only get created for testing. This would be like versioning your bin folder in a c# library. We care about versioning the source code, not the output. In my eyes, the only reason to version anything in the samples folder is so that in a PR, reviewers can see how the output is changing relative to code changes (the output being ts files, not js) without having to build locally.

@Vrolijkx
Copy link
Contributor

@pixelshaded
When we do the bundeling also non Typescript users can use our generated client with the benefit of TypeDefinitions files. And because we know the client will be used on NodeJs only because we use request we are sure they support the CommonJs import syntax.
You are right about your second point. I got mixed up with the angular2Typescript client i was working on.
You are also correct about the last point it would be better to automatically run this on npm prepublish.
But maybe because we only generate one file with the nodeJS TypesScript client the benefit of bundeling and publishing it automatically wouldn't be that great.

@pixelshaded
Copy link
Contributor Author

pixelshaded commented May 25, 2016

Hmmm. I think I must be missing something. Maybe some context? Are we publishing every npm module in samples or something? If so, why? I thought petstore was essentially an example API.

I'm finding it hard to create a scenario in my head where typescript definitions would be useful to a consumer who isn't using typescript. If they aren't using the compiler, they don't have intellisense. There isn't anything linking their javascript client to the typescript code. They would essentially be reading the swagger spec as a typescript definition...why not just read the swagger spec? Why not use other libraries like mentioned in the docs (https://github.com/swagger-api/swagger-codegen#where-is-javascript)? Definitely feels like we are trying to cater to an edge case here. If I was a new user looking for javascript codegen and wasn't using typescript, I feel like targeting tyepscript-node would be the last thing I'd do.

@wing328
Copy link
Contributor

wing328 commented May 27, 2016

@pixelshaded I'll do the cherry-pick over the weekend.

@pixelshaded @Vrolijkx very good discussion. Please keep it going.

@Vrolijkx
Copy link
Contributor

At the moment we don't publish the generated clients but it would be nice if we did. It would be like publishing a generated java client to maven repository and then just add the correct dependency to your consumer and your are ready to use it. This makes it very easy for consumers of your client.

We could do the same thing with npm than a consumer doesn't need to copy paste the generated code in his project but just add the correct npm dependency an he's good to go.
The swagger codegen team could also publish their generated petstore clients to npm this gives interested developers the change to play with the generated code against our working petstore server.
But this are just some of my ideas for the future.

wing328 added a commit that referenced this pull request May 29, 2016
[Typescript-Node] Add Support for useQuerystring Request Option #2905
@wing328
Copy link
Contributor

wing328 commented May 29, 2016

Closed via #2989

@wing328 wing328 closed this May 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.