-
Notifications
You must be signed in to change notification settings - Fork 9k
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
fix: remove special handling of non-FormData entries #6036
Conversation
@@ -86,9 +86,6 @@ export function fromJSOrdered(js) { | |||
const objWithHashedKeys = createObjWithHashedKeys(js) |
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.
The logic here is a little bit nonsensical for me here.
createObjWithHashedKeys
is used in only one place in codebase, and that's here on line 86.
Then first thing the function does is validate it's input param again but with different predicate statement. This seems redundant and non-compatible with isFunction(js.entries)
predicate statement checking if entries
is a function instead checking if it's falsy. I recommend dropping following code from the function body.
if (!fdObj.entries) {
return fdObj // not a FormData object with iterable
}
Another thing about createObjWithHashedKeys
is that it's super crazy complicated. It's not apparent what is does by reading it. So I'd either put @example tag to functions existing jsdoc to document what is the expected output of this function for particular input (formdata) or create a unit tests for this function.
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.
This seems redundant
Internal validation updated to match isFunction
for consistency. Internal validation retained for robustness.
The original use of isFunction(js.entries)
is a conditional, not a validation
put @example tag
Added a real example
unit tests
Not done for 'createObjWithHashedKeys'. Reason: would be new feature with additional dependencies for isomorphic-form-data for testing purposes. This is also the reason for validating fdObj.entries
rather than validating instanceof FormData
.
However, there exists a curlify
test that tests rendering of the result, which is basis of the added @example tag mentioned above
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.
OK everything clear except one thing.
We're trying to detect if the js
is a FormData instance. Why are we not using instanceof FormData
operator but rather asserting on the shape of the possible formData instance? swagger-client
is integral part of swagger-ui and it makes sure that FormData
symbol is always present as global symbol, both in browser and node.js so it's absolutely safe to do so without installing isomorphic-form-data
explicitly.
Reason: would be new feature with additional dependencies for isomorphic-form-data for testing purposes
New feature? I'd rather say it's just a dev-dependency used only for tests. But anyway @examples in jsdoc should suffice if function is tested elsewhere in integration.
de98764
to
bec6aeb
Compare
Ref: swagger-api#6033 * 'createObjWithHashedKeys' validation consistency with isFunction * 'createObjWithHashedKeys' additional jsdoc example
6758abb
to
e9a1487
Compare
Ref: swagger-api#6033 * 'createObjWithHashedKeys' validation consistency with isFunction * 'createObjWithHashedKeys' additional jsdoc example
Ref: #6033
Description
PR #6025 fix to
entries
was over-strict on handlingjs.entries
. The extra handling for anjs
object that may have contained.entries
as a key has been removed. Thejs
object is now processed in its entirety without further modification.Motivation and Context
Fixes #6033
The render failure to expand
Models
was not previously covered in PR #6025.How Has This Been Tested?
Additional tests to expand both the
Operation
andModels
have been added.Screenshots (if appropriate):
Checklist
My PR contains...
src/
is unmodified: changes to documentation, CI, metadata, etc.)package.json
)My changes...
Documentation
Automated tests