⚠️ No Changeset found

Latest commit: d162b6c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@brophdawg11 Snapshots are "failing", but imo the output is exactly the same (even more predictable if you ask me).

So question here mainly is: do we want to update snapshots or do we want to remove these keys?

tl;dr; Can we just use the individual lodash.* utils?

I think the discussion above is a perfect example of why we use tools like lodash- it avoids us having to spend time bikeshedding the "right" way to implement something like pick. This is not mission-critical/super-hot-path code so I really don't see this micro-perf-analysis making any noticeable difference in a users app build times.

Out of curiosity, I ran some tests on this branch and the commit it's branched off of and in my (very unscientific) tests of time npm run build on the react-router-website repo, I received build times of 2.25/1.85/1.84/1.85 seconds when using lodash and 1.98/1.88/1.87/1.89 seconds using these changes. So a minor speedup on initial build and a minor slowdown on subsequent builds (which presumably speed up due to the vite cache). Is that worth kicking a very useful utility like lodash to the curb and writing/maintaining our own utilities instead? Not in my opinion.

Another argument for these types of changes is less for a user to install into their node_modules, which can be a valid argument, but is incorrect in this case since the footprint of es-toolkit appears to be twice the size of lodash:

> du -h node_modules/lodash/
4.9M	node_modules/lodash/
> du -hs node_modules/es-toolkit/
11M	node_modules/es-toolkit/

It does look like lodash is about ~4% of the total size of node_modules for a fresh/minimal create-react-router app (121M) so getting rid of that might have a small impact on install times, so maybe it's worth evaluating how to get rid of it - why not just use the individual lodash.* packages for the utils we need? These take up practically no space, we keep using the same utils, and we slightly reduce the footprint of node_modules for RR apps

> du -hs node_modules/lodash.*
 56K	node_modules/lodash.clonedeep
 64K	node_modules/lodash.isequal
 52K	node_modules/lodash.omit
 28K	node_modules/lodash.pick

In cases where there is a 1:1 platform option (cloneDeep -> structuredClone) I think it's fine to drop the lodash one, but my preference here would be to switch to lodash.isequal/lodash.omit/lodash.pick and call it a day.

Referring back to the the comment on the e18e proposal, in the future lets discuss the pros of these types of changes in a discussion prior to opening PRs so we can decide which we want to move forward with to avoid unnecessary overhead/toil/reviews/perf-testing/etc. I had to spend a chunk of time yesterday evaluating #14111 which came with no data, and another chunk time today evaluating this PR - all of which I would have rather spent on more pressing areas of RR, like working towards stabilizing middleware 😉 . When we do accept a proposal in the future, let's communicate that the PR should come with the raw data demonstrating that we've achieved what we set out to (faster perf, smaller footprint, etc) so that we don't have to run all those numbers ourselves from scratch.

This is not mission-critical/super-hot-path code so I really don't see this micro-perf-analysis making any noticeable difference in a users app build times.

You'll have to forgive me for just playing code golf with my suggestions - however, the Object.fromEntries approach in a microbenchmark ¹ is three times faster than lodash and 100 times faster than the approach in this PR.

Even then, I can appreciate that a potential 10% speedup isn't necessarily justification to move off lodash.

However....

it avoids us having to spend time bikeshedding the "right" way to implement something like pick

I would argue that lodash is rarely the right implementation. It's wildly over-engineered for most use cases (including this one), and for the remaining it's just encouraged poor architecture because it's so flexible.

I'm not sure I'd say es-toolkit is the right implementation either as their pick implementation is still not as optimised as it could be, and doesn't preserve object order.

switch to lodash.isequal/lodash.omit/lodash.pick and call it a day

all the individual packages have actually been deprecated, recommending native alternatives (although the alternatives don't work for omit/pick).

All that being said, none of that rambling is a good enough reason to change.

The motivation for me, as an end(ish) user though is not size over the wire or on disk, or performance - I just like making my lockfile as small as possible, without prejudice. Every additional dependency is a false positive ReDoS, supply chain compromise, or malformed license that'll get me in trouble with legal waiting to happen.

all the individual packages have actually been deprecated

huh, good catch - I didn't know that - that's a bummer but understandable for stuff like map/reduce/etc. that have platform alternatives now, but doesn't make as much sense for things like omit/pick. I must admit I've never actually reached for them them because in my career it's never been an install or perf bottleneck to just use lodash 🙃

I would argue that lodash is rarely the right implementation

It's got 70 million weekly downloads, it's certainly not the wrong choice.

I just like making my lockfile as small as possible

This is totally understandable as a personal preference, but it's just not a pressing need for us at the moment.

As stated in #13721 (comment) though we're not anti-these changes, but we just want to see the real-world impacts outlined so we don't have to try to figure them out ourselves.

For this PR, given we can't use the individual methods:

• I don't think we should bring in es-toolkit here because of it's size
• Is there something else that has the utils we want we can drop in?
• If not, I don't really mind the manual implementations of pick/omit but lets define them as typesafe utils and not write them from scratch each time

It's got 70 million weekly downloads, it's certainly not the wrong choice.

Or an awful lot of wrong choices :P

I agree on not bringing in es-toolkit. The builtin node:util.isDeepStrictEqual should do the trick for isEqual - the test suite passes with it swapped in, though I've obviously no idea of the extent of coverage - and structuredClone for cloneDeep, which leaves pick & omit as a couple of oneliner utils.

It looks like something about cloning is breaking a lot of e2e tests