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

Replace all lodash.* packages #612

Merged
merged 9 commits into from
Sep 13, 2023

Conversation

SukkaW
Copy link
Contributor

@SukkaW SukkaW commented Sep 12, 2023

The PR replaces all lodash.* packages.

  • lodash.escape -> html-escaper
  • lodash.flatten -> a modified version of arr-flatten
  • lodash.debounce -> debounce
  • lodash.pullall -> Array.prototype.filter
  • lodash.uniqby -> Set + Array.prototype.filter
  • lodash.invokemap -> Object.values

Comparison

Running npm i --omit=dev --omit=optional against webpack-contrib/webpack-bundle-analyzer:master:

image

Running npm i --omit=dev --omit=optional against sukkaw/webpack-bundle-analyzer:repalce-lodash-usage:

image

The installation size becomes even smaller!

@SukkaW SukkaW mentioned this pull request Sep 12, 2023
@stof
Copy link

stof commented Sep 12, 2023

For throttle and debounce, I'm using https://www.npmjs.com/package/@github/mini-throttle personally. It is small, efficient and maintained.

@SukkaW
Copy link
Contributor Author

SukkaW commented Sep 12, 2023

For throttle and debounce, I'm using https://www.npmjs.com/package/@github/mini-throttle personally. It is small, efficient and maintained.

I was thinking about using perfect-debounce from the unjs community (https://www.npmjs.com/package/perfect-debounce). However, webpack-bundle-analyzer still targets Node.js 10.13.0+ and I was afraid of breaking it. So I chose the debounce package instead. Once the Node.js 10 support has been dropped, we can switch to a more modern package.

@SukkaW SukkaW marked this pull request as ready for review September 12, 2023 16:14
@SukkaW SukkaW changed the title (WIP) Drop all lodash.* packages Replace all lodash.* packages Sep 12, 2023
@stof
Copy link

stof commented Sep 13, 2023

Do you also want to replace the packages used as dev dependencies ?

@valscion
Copy link
Member

Do you also want to replace the packages used as dev dependencies ?

Hmm, I don't know about that. If we do decide to go that route, let's at least split that work to its own PR to make this diff smaller.

Copy link
Member

@valscion valscion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me!

Any changes you want to add or is this ready to go?

EDIT: Oh right, I forgot about that packageManager comment I had pending. That would need to be resolved first.

@@ -18,6 +18,7 @@
"engines": {
"node": ">= 10.13.0"
},
"packageManager": "npm@6.14.8",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm if I've understood the corepack docs correctly, having a packageManager with npm in it is of no use.

Is this needed for some purpose other than that?

Copy link
Contributor Author

@SukkaW SukkaW Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@valscion

By default npm is already shipped with Node.js, so you don't need corepack to install npm, that's why the docs said something of "no use". But, the field still helps when corepack is enabled and a specific version of npm is needed.

I have been using the latest version of npm (which uses the lockfile v2/v3) for a while. However, the webpack-bundle-analyzer still uses the v1 version of package-lock.json and I need to use an older version of npm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay cool! I didn't know that this allowed corepack to use an older version of npm. This is useful!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on https://nodejs.org/api/corepack.html#how-does-corepack-interact-with-npm, I don't think corepack will install an older npm version

Copy link
Contributor Author

@SukkaW SukkaW Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on https://nodejs.org/api/corepack.html#how-does-corepack-interact-with-npm, I don't think corepack will install an older npm version

But for some reason, it does work for me though. I am able to use the 6.x version of npm to generate the correct package-lock.json that won't bloat the change.

And if I cd into other projects the npm automatically reverted back to the latest version.

Copy link
Contributor Author

@SukkaW SukkaW Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof I might have found out why corepack works for me. It might have something to do with my special setup:

  • I use fnm, a tool like nvm, to manage my Node.js versions
  • Though not recommended, I've manually specified NPM_CONFIG_PREFIX so npm will install global packages to my specific location (instead of the default location)
  • I've upgraded my npm version with npm i npm -g, so my npm is located inside NPM_CONFIG_PREFIX instead of the default location.
  • In my $PATH, I add the Node.js path before the NPM_CONFIG_PREFIX, so the shell will choose npm from corepack when available.
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so a special setup is the reason.

I don't think having the packageManager line hurts in any way, though. Or do you think it's a bad idea to have in the repository?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so a special setup is the reason.

I don't think having the packageManager line hurts in any way, though. Or do you think it's a bad idea to have in the repository?

Yes, it doesn't hurt.

This packageManager field has no effect on the package's users. Maintainers/contributors who don't have corepack won't be affected either. Maintainers/contributors who have enabled corepack without a specific setup will have an extra copy of corepack's npm installed, it just won't be chosen by the shell (The shell will prefer the original npm without the setup).

@valscion valscion merged commit f01056a into webpack-contrib:master Sep 13, 2023
5 checks passed
@valscion
Copy link
Member

Released in v4.10.0! ☺️

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

Successfully merging this pull request may close these issues.

3 participants