swagger-js
swagger-js copied to clipboard
swagger-api
Try to remove lodash?
https://github.com/swagger-api/swagger-js/blob/3937607d80879c4627a780e7b3a94c1c13ebed48/src/http/index.js#L4-L5
// Native pick
const { a, c } = object;
const result = { a, c };
// native is fn
typeof x === 'function'
https://github.com/swagger-api/swagger-js/blob/22da4ad9bbe9ea742ad8a20b15f7c10160bb3043/src/execute/oas3/build-request.js#L3-L4
Object.assign(...)
const securityDef = spec?.components?.securitySchemes || {}
Hi,
Using lodash over native JS and vice versa is highly debated topic lately. Some of the articles I read lately on the topic includes:
- https://dev.to/jrdev_/the-battle-of-the-array-titans-lodash-vs-vanilla-an-experiment-b6g
- https://areknawo.com/lodash-and-usefulness-of-utility-libraries/
- https://thejs.dev/jmitchell/its-time-to-let-go-of-lodash-nqc
- https://codeburst.io/why-you-shouldnt-use-lodash-anymore-and-use-pure-javascript-instead-c397df51a66
- https://blog.bjoern-ruberg.de/2018/10/06/why-you-should-still-embrace-lodash/
My take on this topic is:
It depends on the context...If there is a native JS alternative in this code-base we should probably use it. Code in this code-base is highly imperative anyway, so converting a declarative approach of using utility function from lodash into imperative native JS counterparts wouldn't do any harm. Feel free to issue PRs.
Note on predicates:
I would still prefer the use of predicate function, such as isFunction, isString, isNumber and so on. They either come from lodash or are defined locally in utility module. The reason I prefer them is that they are
- self-contained
- already predefined, tested and cover corner cases
- low risk
By using typeof x === 'function' there is too much going on here IMHO, and too much things can go wrong here as well. We can make a simple typo in function string very easily and get an unexpected program results.
typeof x === 'function' vs isFunction(x)
@jimmywarting would you like to own this issue and implement the proposed changes?
As part of this issue, pick has been replaced with object destructuring. @jimmywarting nice job!
as part of trying to remove lodash/find i notice this IE11 comment
nextPlugin() {
// Array.prototype.find doesn't work in IE 11 :(
return find(this.wrappedPlugins, (plugin) => {
do you still support IE at all? or do you use something lite corejs that polyfills missing features or something like that?
Object.assign got merged but it's not supported in ie11 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign
We define that very precisely here: https://github.com/swagger-api/swagger-js#runtime. IE is not part of that list.
Theoretically your change still might work in IE if IE 11 currently falls within this browserlist:
> 1%
last 2 versions
Firefox ESR
not dead
We do apply polyfills given above browserlist using babel-env and @babel/plugin-transform-runtime
left:
- [x] isObject
- [x] isPlainObject
- [ ] get
- [x] isEmpty
- [ ] cloneDeep
The rest is kind of annoying...
get could be replaced with "optional chaining" unless they are a dynamic array or use weird features that get has
(depends on the context)
I found this package https://www.npmjs.com/package/is-empty, do you think we can replace lodash/isEmpty with it?
@gabrielsimongianotti we have only one case of using the isEmpty function here: https://github.com/swagger-api/swagger-js/blob/e5788a8d1233b6cbbc0821d43392393c5a6ec607/src/specmap/lib/all-of.js#L38
We're specifically asserting if the plain object is not an empty one. IMHO we can replace that isEmpty functiona with Object.keys(originalDefinitionObj).length === 0 in that particular case.
Anybody would want to take care of get? Just not to loose the momentum with this issue.
There is a regression in introduced in https://github.com/swagger-api/swagger-js/commit/067229e8f7d6b55a68cb62278a662fc2a97c9f0a described in https://github.com/swagger-api/swagger-ui/issues/7771 that manifests in SwaggerUI
Is there a branch I can use, where this is implemented. I am running into the issue described here: https://github.com/swagger-api/swagger-js/issues/2439
@transfluxus nope there is not. We've working on this progressively. If you want to help, you can work directly on master.
:tada: This issue has been resolved in version 3.21.0 :tada:
The release is available on:
Your semantic-release bot :package::rocket:
