[e18e]: Replace lodash/merge & lodash/mergeWith with an alternative

[xeho91]

This is a sub-partial work for tracking-issue #28611.

Just an idea.
Perhaps we could do one branch, derived from next e.g. e18e, where we could put all of the work related to replacing lodash with alternatives?
It could possibly be easier to show how significant change on the summarised work of replacing lodash impact on bundle
size as well as performance.

What I did

Replaced lodash/merge and lodash/mergeWith with deepmerge-ts.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

I am not entirely sure if there's no breaking visual changes in the UI. After the changes to:

• merge snippet - I couldn't find tests specific to this snippet, nor explore in depth where it is being used
• combineParameters() - existing tests passed

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for Maintainers

• When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts

• Make sure this PR contains one of the labels below:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

[tobiasdiez]

An alternative to deepmerge-ts would be https://github.com/unjs/defu, which allows a bit more control on how to merge two objects.

[xeho91]

An alternative to deepmerge-ts would be https://github.com/unjs/defu, which allows a bit more control on how to merge two objects.

Thanks, I haven't heard about this one!

I'm not sure what you had in your mind with 'more control'? By quick glance deepmerge-ts has more extensive API. And I want to make it crystal clear - I'm not willing die on hill favouring this one, and willing to switch to defu if needed.

To compare quickly:

-	deepmerge-ts	defu
bundle size	👎 5.8kB	👍 1.2kB
dependencies	👍 0	👍 0
API	very extensive	simple
learning curve	👎 hard if used deepmergeCustom	👍 easy with createDefu
performance	👎 benchmark	👍benchmark

From what I've observed while working on this PR.
I didn't see any need for the "advanced" usage with the need for a customized merger other than merge snippet:

• original: https://github.com/xeho91/storybook/blob/next/code/core/src/manager-api/lib/merge.ts
• after changes: https://github.com/xeho91/storybook/blob/bye-lodash-merge/code/core/src/manager-api/lib/merge.ts

[tobiasdiez]

I don't have a strong opinion either, just wanted to drop this alternative because it worked for me (and is the go-to solution in the nuxt ecosystem). But as you said the usage of merging objects in storybook is quite simple, so it doesn't really matter.

[xeho91]

I've created an initial benchmark PR on deepmerge-ts repo.

Looks like defu is a clear winner in terms of performance.

[valentinpalkovic]

Hi @xeho91

I think it definitely makes sense to create a base branch to merge all the changes in because, in the beginning, the bundle size will be increased until we are able to remove the lodash library. I have created the branch e18e-lodash.

[valentinpalkovic]

I guess let's use defu then, and let's take a deeper look at adding some unit tests.

Maybe we could reuse/copy over the unit tests from lodash (merge, merge-ts and mergeWith to understand in which aspects the libraries are behaving differently so that we can make a solid assessment.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit c28f30b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

---

✅ Successfully ran 1 target

• nx run-many -t build --parallel=3

---

Sent with 💌 from NxCloud.

[xeho91]

I guess let's use defu then, and let's take a deeper look at adding some unit tests.

Maybe we could reuse/copy over the unit tests from lodash (merge, merge-ts and mergeWith to understand in which aspects the libraries are behaving differently so that we can make a solid assessment.

About the unit tests - should we put them in merge.test.ts:

1. in the same directory as merge.ts
2. or is there any different location the test file should be placed?

[xeho91]

I've had to merge next to this branch, because I've had issues with PreviewWeb tests snapshots and using defu. They started to pass once I merged latest changes.

Hence e18e-lodash branch might need merging next, so we can exclude changes unrelated to this PR.

[xeho91]

I need help with decision on this one.

I've had issues with types mismatch, when the records from the second argument had types which did not exists on the first argument (source) in the PreviewWeb.test.ts.

Is it okay to change type of second argument from Partial<TObj> to Partial<TObj> & any?

[valentinpalkovic]

Sure. Just widen the types for now and we can take a proper look afterwards.

[valentinpalkovic]

I guess let's use defu then, and let's take a deeper look at adding some unit tests.
Maybe we could reuse/copy over the unit tests from lodash (merge, merge-ts and mergeWith to understand in which aspects the libraries are behaving differently so that we can make a solid assessment.

About the unit tests - should we put them in merge.test.ts:

1. in the same directory as merge.ts
2. or is there any different location the test file should be placed?

Yes, let's put them in merge.test.ts

[JReinhold]

Looks like the formatting in this file got borked?

[JReinhold]

FYI, even though we just merged #28981 to replace lodash with es-toolkit, that shouldn't block this PR from moving forward, because I still think the approach in this PR is an improvement (better and smaller) than the solution we've just merged.

💪

[xeho91]

I somehow unsubscribed myself from the notifications for PR. 😟

If we still can move forward with using defu for merging, should I use the same target branch or the next?
Asking in advance, so I don't spend too much time on resolving existing conflicts.

[JReinhold]

Good question, I'd say target next directly here. Perhaps that other target branch is now entirely superseded by our done lodash migration @valentinpalkovic ?