refactor(dev): replace lodash

[MichaelDeBoey]

Alternative to #14111

---

I'm sure people from the wider @e18e ecosystem cleanup (like @43081j, @Fuzzyma & @outslept) will be very happy to see these kind of changes as well

[changeset-bot]

⚠️ 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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

[MichaelDeBoey]

@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?

[brophdawg11]

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.

[mrginglymus]

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.

Footnotes

1. yes it's a microbenchmark, you try soak testing a cli... ↩

[brophdawg11]

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

[mrginglymus]

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.

[brophdawg11]

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

[mrginglymus]

The tests, at least on my machine, appear to run fine individually. I also appear to be getting similar failures on main.

[43081j]

what error is this suppressing? 👀

[MichaelDeBoey]

excludedConfigPresetKeys will always be ["presets"], which technically can't happen according to the types.
So in order to remove presets property from preset.reactRouterConfig's return and the fact that omit is now type-safe, there's no other way than suppressing this error, otherwise TS will yell at us.

@brophdawg11 Looking at the code, I would say we could just map to preset.reactRouterConfig's return value as according to the type of preset, we'll always have a name & reactRouterConfig (which doesn't have a presets property, so omit can be removed as well purely type-wise)

[43081j]

because a ConfigPreset doesn't have a presets key, right?

yeah thats awkward. you can cast as never, or, safer, you could cast the source as ConfigPreset & {presets?: unknown}

personal preference i guess, though

[MichaelDeBoey]

because a ConfigPreset doesn't have a presets key, right?

Indeed.

you can cast as never

Yeah I don't like casting, hence the expect-error, but I guess @brophdawg11 can just tell me the preference of the team and I'll update to whatever is preferred

[brophdawg11]

I'll defer to @pcattori for type preference questions

[pcattori]

The expect-error is nice because we'll get a type error if/when we fix types for ConfigPreset, at which point we can clean this up. Whereas with a cast, that cast will likely live in the codebase forever.

[MichaelDeBoey]

@pcattori That's indeed (one of) the reason(s) why I don't like casting.

[43081j]

the tests fail because configs can contain functions. particularly this test fixture:

react-router/integration/vite-presets-test.ts

Line 37 in 75ab018

reactRouterConfig: async ({ reactRouterUserConfig: { presets, ...restUserConfig } }) => {

we're currently cloning the options to avoid mutating them - this probably means we are holding 2 copies of the object in memory each time, which isn't the best.

i wonder if we can achieve what we want through types rather than cloning the object (i.e. readonly).

if this object gets passed to user code (i.e. a user/plugin/preset/etc can mutate it), that won't fly. but if it never leaves the bounds of react-router itself, we could probably do it.

if it turns out we do need to deep clone, we could use klona

[mrginglymus]

There are only two uses of deepFreeze - here, and on a newly created object.

Given that deep freeze is already recursing the object, it seems like folding the cloning into that would potentially simplify things a bit (as well as saving a few cycles).

[brophdawg11]

For the time being I'm going to close this so we an continue to discuss the proper solution in #13721 (comment). Once we've landed on a valid solution there we can evaluate it and re-open a PR.