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

Get rid of lodash and swap it out for lodash-es #2311

Open
fabis94 opened this issue Apr 2, 2024 · 6 comments
Open

Get rid of lodash and swap it out for lodash-es #2311

fabis94 opened this issue Apr 2, 2024 · 6 comments

Comments

@fabis94
Copy link

fabis94 commented Apr 2, 2024

Is your feature request related to a problem? Please describe.

The CJS build of lodash is old and not very well optimized for web use cases (hard to tree-shake). So far I've been able to alias lodash to lodash-es in my build config, but then you recently introduced calls to lodash/fp which basically broke all of that.

That was a bad call IMO and just made it more of a chore to get your library working in web environments, especially if you don't want to bundle in all of lodash.

Describe the solution you'd like

Get rid of lodash usages and swap them all out for lodash-es

Describe alternatives you've considered

Doesn't have to be lodash-es necessarily, just something new that has a proper tree-shakeable ESM build

Framework

Vue

RendererSet

Vanilla

Additional context

No response

@lucas-koehler
Copy link
Contributor

Hello @fabis94 , thanks for suggesting this improvement.

just made it more of a chore to get your library working in web environments

Do you have concrete problems that you cannot get it to work at all or is your "only" problem that you do not want to bundle lodash to save on bundle size?

As I understand it, there would be no need (for you) to replace lodash with lodash-es if there were not calls to lodash/fp?
Can the fp functions generally still be used when using the lodash-es build? Or would this require rewriting the code to use other functions?

@fabis94
Copy link
Author

fabis94 commented Apr 5, 2024

@lucas-koehler lodash-es does not have these fp functions, at least under those imports so my previous method of just aliasing all lodash/** imports to lodash-es/** does not work because of this library. It did work previously however with all kinds of other libraries and dependencies (running a Nuxt app).

I suppose you're right, this is only caused because of trying to optimize on lodash's size, but I would argue that its an important concern. Nowadays you have to care about things like that (ESM support, tree-shaking etc.) to ensure good UX for users.

And I think you may be right that if calls to lodash/fp are removed, I might still be able to do all of that aliasing, cause I remember not having this issue before those calls were introduced. But if its that easy, maybe its just as easy to swap all of the lodash calls to lodash-es inside of this library directly.

@lucas-koehler
Copy link
Contributor

@fabis94 It seems that the fp part of lodash is not available in the lodash-es build. See this issue of lodash: lodash/lodash#5285

Generally, it seems reasonable to me to remove the usage of lodash/fp and, thus, facilitate aliasing lodash with lodash-es in a first step. Later we could then change this dependency from lodash to lodash-es.
Would you be willing to contribute the removal of using fp? @fabis94

Currently lodash/fp seems to be used only in:

  • packages/core/src/reducers/core.ts
  • packages/vanilla-renderers/src/complex/TableArrayControl.tsx

@sdirix Do you see a reason why we could not get rid of using lodash/fp or move to lodash-es longer term.

@fabis94
Copy link
Author

fabis94 commented Apr 5, 2024

@lucas-koehler I'm not very knowledgeable about jsonforms and lodash/fp, so I'd prefer not to, but if there's no other way I supposed I could try taking a look at some point

@lucas-koehler
Copy link
Contributor

@fabis94 Alright. I'll discuss the priority of this with the team. However, as this is merely an optimization I cannot promise if/when we will work on this. If you start to work on this, please leave a comment here :)

@sdirix
Copy link
Member

sdirix commented Apr 8, 2024

Current usage

The reason we are using lodash/fp/set in core is because we need to emit an updated data object in which all parents of the modified attribute in the hierarchy tree are reconstructed, while avoiding to create copies for all (nested) sibling/cousin properties. This is exactly what lodash/fp/set provides to us.

Of course we could get rid of this as long as we can use/implement a similar alternative.

The use in vanilla-renderers is historic. Lodash/fp was very popular during the initial creation of the Vanilla renderer set and was therefore used extensively there. Over time some of this was refactored but some usages remained. Similar to the other usage in core, this is "just effort" to get rid of which must be tackled by someone at some point.

Lodash usage / Tree shaking etc.

To improve bundle size we are already using deep imports, e.g. import get from 'lodash/get' instead import { get } from 'lodash'. This "manual tree shaking" should already provide most of the wins you get from lodash-es. @fabis94 Did you do an analysis on how much the bundle size improves when using lodash-es compared to lodash in case you are only using JSON Forms and not have other named imports from lodash in your code base?

In client projects we observed that using deep imports already made sure that most of lodash was not included in the end.

Using lodash-es

lodash is still far more popular than lodash-es. If we now change our usage to lodash-es then every consumer using regular lodash will then actually have lodash twice in their bundle, once from their own lodash usage and then lodash-es coming in via JSON Forms.

While users purposefully using lodash-es probably are already aware of aliasing tricks and can just continue to use that.

My thoughts

  • I don't have any issue with refactoring the usage of lodash/fp with regular lodash as long as we don't introduce regressions. Contributions welcome
  • To refactor the code base to use lodash-es over deep imported lodash I would like to see an analysis that this actually improves bundle size

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

No branches or pull requests

3 participants