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

Try to remove lodash? #2187

Closed
jimmywarting opened this issue Aug 12, 2021 · 16 comments · Fixed by #3137
Closed

Try to remove lodash? #2187

jimmywarting opened this issue Aug 12, 2021 · 16 comments · Fixed by #3137

Comments

@jimmywarting
Copy link
Contributor

jimmywarting commented Aug 12, 2021

import pick from 'lodash/pick';
import isFunction from 'lodash/isFunction';

// Native pick
const { a, c } = object;
const result = { a, c };

// native is fn
typeof x === 'function'

import assign from 'lodash/assign';
import get from 'lodash/get';

Object.assign(...)
const securityDef = spec?.components?.securitySchemes || {}
@char0n
Copy link
Member

char0n commented Aug 13, 2021

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:

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)

@char0n
Copy link
Member

char0n commented Sep 10, 2021

@jimmywarting would you like to own this issue and implement the proposed changes?

@jimmywarting
Copy link
Contributor Author

sure thing

@char0n
Copy link
Member

char0n commented Sep 13, 2021

As part of this issue, pick has been replaced with object destructuring. @jimmywarting nice job!

char0n pushed a commit that referenced this issue Sep 13, 2021
- replace lodash.isArray for native Array.isArray
- replace lodash.assign for native object destructuring or Object.assign

Refs #2187
@jimmywarting
Copy link
Contributor Author

jimmywarting commented Sep 13, 2021

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

@char0n
Copy link
Member

char0n commented Sep 13, 2021

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

@jimmywarting
Copy link
Contributor Author

jimmywarting commented Sep 15, 2021

left:

  • isObject
  • isPlainObject
  • get
  • isEmpty
  • cloneDeep

The rest is kind of annoying...

char0n pushed a commit that referenced this issue Sep 15, 2021
- startsWith
- noop
- isFunction
- isString
- find

Refs #2187
@jimmywarting
Copy link
Contributor Author

get could be replaced with "optional chaining" unless they are a dynamic array or use weird features that get has

(depends on the context)

@gabrielsimongianotti
Copy link
Contributor

I found this package https://www.npmjs.com/package/is-empty, do you think we can replace lodash/isEmpty with it?

@char0n
Copy link
Member

char0n commented Nov 10, 2021

@gabrielsimongianotti we have only one case of using the isEmpty function here:

if (isEmpty(originalDefinitionObj)) {

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.

char0n pushed a commit that referenced this issue Nov 11, 2021
@char0n char0n removed their assignment Dec 14, 2021
@char0n
Copy link
Member

char0n commented Dec 14, 2021

Anybody would want to take care of get? Just not to loose the momentum with this issue.

@char0n
Copy link
Member

char0n commented Jan 19, 2022

There is a regression in introduced in 067229e described in swagger-api/swagger-ui#7771 that manifests in SwaggerUI

@transfluxus
Copy link

Is there a branch I can use, where this is implemented. I am running into the issue described here:
#2439

@char0n
Copy link
Member

char0n commented May 15, 2023

@transfluxus nope there is not. We've working on this progressively. If you want to help, you can work directly on master.

@char0n
Copy link
Member

char0n commented Sep 12, 2023

Lodash removed in #3137

@char0n char0n closed this as completed Sep 12, 2023
@char0n char0n self-assigned this Sep 12, 2023
char0n added a commit that referenced this issue Sep 12, 2023
swagger-client requires Node.js >=12.20.0 and uses different fetch 
implementation depending on Node.js version:

>=12.20.0 <16.8 - node-fetch@3
>=16.8 <18 - undici
>=18 - native Node.js fetch

Closes #1220
Closes #2736
Closes #2415
Closes #2381
Closes #2187
Closes #2291
swagger-bot pushed a commit that referenced this issue Sep 12, 2023
# [3.21.0](v3.20.2...v3.21.0) (2023-09-12)

### Features

* replace node-fetch with undici ([#3137](#3137)) ([bc7ca17](bc7ca17)), closes [#1220](#1220) [#2736](#2736) [#2415](#2415) [#2381](#2381) [#2187](#2187) [#2291](#2291)
@swagger-bot
Copy link
Contributor

🎉 This issue has been resolved in version 3.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

5 participants