-
Notifications
You must be signed in to change notification settings - Fork 5
feat: support application/x-www-form-urlencoded request bodies #352
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
Conversation
| import {ClientServersBuilder} from "./client-servers-builder" | ||
|
|
||
| export abstract class AbstractClientBuilder implements ICompilable { | ||
| protected abstract readonly capabilities: { |
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.
Rudimentary capabilities concept to more easily allow for skew of feature parity between templates
| style: "spaceDelimited", | ||
| // note: undefined by spec | ||
| string: "color=blue", | ||
| // TODO: is it correct to use + for spaces here? |
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.
There's several test cases here using examples form the OAI specification that don't agree. I've opened OAI/OpenAPI-Specification#4813 to discuss
| }, | ||
| ) | ||
|
|
||
| describe("stripe /v1 api conventions; style: 'deepObject' explode: true", () => { |
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.
Where behavior is otherwise undefined in terms of OAI, I've aligned to what the stripe v1 API does/expects. It's a popular API, the only example of nested objects/arrays I have right now.
1ec1c0a to
0449e27
Compare
|
|
||
| const queryString = builder.queryString() | ||
| const headers = builder.headers() | ||
| const headers = builder.headers({nullContentTypeValue: "false"}) |
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.
Turns out that axios will always send a Content-Type header with POST requests, even if there isn't a body. This then ends up being parsed by the receiving server erroneously.
Setting the Content-Type header to false is the only way I've found to drop this behavior 😢 (it then omits the header completely, setting to null or undefined don't work)
| return serialize | ||
| } | ||
|
|
||
| return `${param} !== undefined ? ${serialize} : null` |
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.
Avoids issues with exact optional types compiler option - RequestInit defines body?: T | null so undefined isn't accepted with exactOptionalPropertyTypes
| return headers | ||
| } | ||
|
|
||
| protected _requestBodyToUrlSearchParams( |
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.
In two minds about this, but using a protected wrapping method leaves the door open to someone extending the client class and overriding it I guess
| if (body !== "disabled") { | ||
| app.use(express.json(body?.json)) | ||
| app.use(express.text(body?.text)) | ||
| app.use(express.urlencoded(body?.urlencoded ?? {extended: true})) |
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.
I'm starting to sense a future where we switch to using https://www.npmjs.com/package/raw-body by default for greater control over the parsing (being able to respect encoding properly)
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.
Duplicates the fetch runtime code for now - will introduce a shared package as a follow up
d1a25a1 to
ca58419
Compare
adds support for
application/x-www-form-urlencodedrequest bodies, including theencodingproperty to customize the serialization of object and array values.it's implemented for
typescript-fetchandtypescript-axioswith partial support on the server templates (they don't currently understand encoding, so its a bit of pot-luck whether they'll accurately de-serialize the data)later we'll extract some of the duplicated code into a new core runtime package, as there's an increasingly large body of code that is shared between the different runtimes.
additionally improves handling of optional request bodies, particularly in the case that the consuming project has
exactOptionalPropertyTypesenabled. This is currently more correct on thetypescript-fetchtemplate, due to some quirks ofaxios- see skippede2etests for details.