MPDX-6917-Remove-Lodash by TheNoodleMoose · Pull Request #8 · CruGlobal/mpdx-react

TheNoodleMoose · 2021-02-15T22:28:28Z

No description provided.

…nd tests

vercel · 2021-02-15T22:28:31Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/cru/mpdx-react/h9xpzaxts
✅ Preview: https://mpdx-react-git-mpdx-6917-remove-lodash.cru.vercel.app

TheNoodleMoose · 2021-02-15T22:33:28Z

@@ -0,0 +1,5 @@
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+const omit = (keysToOmit: string[], originalObj = {}): any =>


I had the type for this as Record<string, unknown> and it worked everywhere except for one spot that was using a generated type that was expecting subject. Switched to any for now until I could figure out how to resolve it.

It probably should use generics. I can help play with it if you want.

Or we could remove this helper function completely and use destructuring to pull out keys we want to ignore.

Yeah I started to go the descructuring route but was unsure what to do with the values we omitted(since they aren't going to be used in most cases). I believe there's a lint rule we could throw in to ignore unused values but wanted to get your opinion on that?

I think a naming convention is to prefix them with an _. I assume there's a lint rule to allow that. Although that's a little odd in destructuring cuz you have to rename them probably const { key: _key } = ...;. Idk what's nicest...

We could keep lodash or something similar for this or other use cases, it just seemed a little overused.

https://stackoverflow.com/a/56157826/665224 suggests varsIgnorePattern (and ignoreRestSiblingsbut that seems like the less strict option).

Yeah, varsIgnorePattern worked so I went for the destructuring route!

…lint rule

TheNoodleMoose · 2021-02-16T17:01:20Z

@OzzieOrca I ran into some issues with the Form.test.tsx test. One moment they would pass, I'd make a small change and then it would throw an Timeout - Async .... error. What fixed it was running the test by itself and then running all the tests again.

…ntList component

wrandall22 · 2021-02-16T22:13:30Z

-
-        return initialFilter;
+        const { userIds, tags, contactIds, activityType, completed, wildcardSearch, startAt } = initialFilter;
+        const finalInitalFilter = { userIds, tags, contactIds, activityType, completed, wildcardSearch, startAt };


Seems like this const is unnecessary? Couldn't we just return this?

Yep, will change!

OzzieOrca · 2021-02-16T22:32:12Z

Ya that test has issues... That's the one I increased timeouts on. You could increase the timeout more or just hope it finishes on CI. We probably need to spend some time on that one...

TheNoodleMoose · 2021-02-17T19:20:51Z

@OzzieOrca So I removed the majority of the lodash functions I believe could be easily replaced with native options. It definitely brought the number of lodash usages down. I left things like cloneDeep, uniqBy, curry, and a few spots where reduce is used.

I also tried replacing the lodash reject with the native implementation but for some reason, it caused a timing out issue with the test 😕 Let me know what you think! Bill also suggested looking into if it's possible to only import the functions we care about instead of the whole lodash library?

# Conflicts: # .eslintrc.js # pages/accountLists/[accountListId]/tasks.tsx # src/components/App/Provider.tsx # src/components/Layouts/Primary/TopBar/NotificationMenu/Item/Item.tsx # src/components/Layouts/Primary/TopBar/TopBar.tsx # src/components/Task/Drawer/CommentList/CommentList.tsx # src/components/Task/Drawer/CommentList/Item/Item.tsx # src/components/Task/Drawer/ContactList/ContactList.tsx # src/components/Task/Drawer/ContactList/Item/Item.tsx # src/components/Task/Drawer/Form/Form.mock.tsx # src/components/Task/Drawer/Form/Form.tsx # src/components/Task/List/List.tsx # src/components/Task/Status/Status.tsx # src/lib/intlFormat/intlFormat.ts # src/lib/reduceObject/reduceObject.test.ts

OzzieOrca

I'm ok with leaving some lodash usage if it seems to really provide value but I don't think we should be reaching for it all the time...

Those cloneDeeps could probably be replaced by lots of spreads... We've done that in MissionHub for Apollo cache updates... Apollo might have a better way to do that now.
reject should be a `filter with the opposite condition right?
With uniqBy maybe we could do something with JS Sets but that might be more complicated. Maybe that could live in a helper file...

Lodash should be able to support tree shaking... Maybe we need a babel or webpack plugin... Something is still including the big lodash bundle...

(I've been meaning to add a build:analyze npm script for this but I got this from running ANALYZE=true yarn build)

OzzieOrca · 2021-02-19T01:56:16Z

            break;
          default:
-            result[key.replace('[]', '')] = castArray(value);
+            result[key.replace('[]', '')] = Array.isArray(value)


I know you didn't write any of this but it's some weird code...

The reduceObject around this is the only usage of reduceObject... And the current implementation throws away all types with the cast to any. Maybe we should just remove it?

We could probably use an Object.entries(filter).reduce((result, [key, value]) => {...}, {}).

I'm not sure I understand this default case... I think using the bracket arrayFormat on the parse https://github.com/sindresorhus/query-string#arrayformat might clean up the [] in the key but looks like it although it always returns an array, the types don't know that...

OzzieOrca · 2021-02-19T02:30:42Z

-                { user: { id } },
-                filterData.accountListUsers.nodes,
+              const accountListUser = filterData.accountListUsers.nodes.find(
+                ({ user: { id: accountListUserId } }) =>


Are we sure these objects are defined? Maybe the types say so... Lodash usually swallows undefined errors in cases like this.

Yeah, based on the generated type user should always be defined and same with id. However, the user firstName and lastName could be null. Should we have it check that the name(at least the firstName) is a non-null value as well?

Sounds like it's not going to throw a can't read property of undefined error then. I think the names are ok unless we really need a perfectly empty string if there are no names.

So it uses the firstName and lastName to render the filter:

return `${accountListUser.user.firstName} ${accountListUser.user.lastName}`

So if those are both null it would just render a string of null null... but at the same time, it would be very unlikely an account wouldn't have a first and last name right?

Oh ya for some reason I thought the null would turn into an empty string but it doesn't. I have no idea what the DB constraints are...

TheNoodleMoose · 2021-02-19T18:20:09Z

For the reject I replaced it with this and confirmed it worked as expected. But the test suite for Form.test.tsx fails to run and throws a Call retries were exceeded error.

data.task.comments.nodes = [
     ...data.task.comments.nodes.filter(
    ({ id: commentId }) => id !== commentId,
    ),
    comment,
];

…leCompare, switch to nullish coalescing operator

TheNoodleMoose · 2021-02-19T20:29:51Z

+                ...reject(
+                  ({ id: commentId }) => id === commentId,
+                  dataFromCache.task.comments.nodes,
+                ),


Still couldn't get the filter method I created to replace this reject to not break the test suite. It works fine in the browser but not in the tests for some reason.

- Avoid importing the big lodash.min.js

OzzieOrca

I think this is pretty good for now. Thanks for cleaning stuff up :) Maybe we can tackle some of the weirder ones eventually.

I added a commit to import from lodash/fp/<function name> instead of all of lodash/fp since that seemed to remove the big lodash.min.js file and save 17.77KB (gzipped). So now lodash is just using 16.43KB. We could go back to the big import if we configure some lodash tree shaking plugin for webpack or babel but I think this is good for now.

TheNoodleMoose · 2021-02-19T22:18:29Z

@OzzieOrca Sounds good, thanks for the help 🙂 I also made a new ticket of a bug I discovered while working on this. I was going to include it here but figured it would be better to be in a separate pr.

Flows columns min-width is 300px. Adding/Removing columns now updates local as well as server. Unused Statuses now sticks to the right-hand-side, to make it easier for users to move statuses around. Fixed :hover of the coloured circles. useDrops and useCallbacks to include flowOptions as a dependency. add column button loading for UX as sometimes it would take a while to add a new column. Added Tests MPDX-7614-MPDX-7615 p# This is the commit message #8:

TheNoodleMoose added 3 commits February 15, 2021 10:57

Remove lodash compact, isFunction, isFinite, and isNil

f7588ff

Remove lodash sortBy and find

84be79b

Remove lodash omit and pick functions, create onative omit function a…

6de7698

…nd tests

TheNoodleMoose added the wip Work in Progress label Feb 15, 2021

TheNoodleMoose self-assigned this Feb 15, 2021

TheNoodleMoose commented Feb 15, 2021

View reviewed changes

Use object destructuring to omit values, remove omit function, add es…

89abca0

…lint rule

vercel Bot deployed to Preview February 16, 2021 16:58 View deployment

Remove lodash castArray and remove, switch to native reduce for comme…

a3e7545

…ntList component

vercel Bot deployed to Preview February 16, 2021 22:11 View deployment

wrandall22 reviewed Feb 16, 2021

View reviewed changes

remove const in favor of just returning object

5b7d8af

vercel Bot deployed to Preview February 17, 2021 18:18 View deployment

TheNoodleMoose removed the wip Work in Progress label Feb 17, 2021

vercel Bot deployed to Preview February 19, 2021 01:44 View deployment

OzzieOrca suggested changes Feb 19, 2021

View reviewed changes

Remove reduceObject function, remove lodash cloneDeep, switch to loca…

bf0d738

…leCompare, switch to nullish coalescing operator

vercel Bot deployed to Preview February 19, 2021 20:25 View deployment

TheNoodleMoose commented Feb 19, 2021

View reviewed changes

Remove concat

5ee768c

vercel Bot deployed to Preview February 19, 2021 21:22 View deployment

Import specific lodash modules only

e4cbdc3

- Avoid importing the big lodash.min.js

vercel Bot deployed to Preview February 19, 2021 21:55 View deployment

OzzieOrca approved these changes Feb 19, 2021

View reviewed changes

TheNoodleMoose merged commit 53826f5 into main Feb 22, 2021

TheNoodleMoose deleted the MPDX-6917-Remove-Lodash branch February 22, 2021 14:07

cru-atlantis unassigned TheNoodleMoose Mar 14, 2022

renovate Bot mentioned this pull request May 17, 2023

Update dependency node to 18.18 #532

Closed

1 task

canac added a commit that referenced this pull request Oct 7, 2024

Rerun tests #8

b473eb8

zweatshirt mentioned this pull request Apr 16, 2026

MPDX-9367 - PDS Calc Salary Subtotal section #1721

Merged

6 tasks

		@@ -0,0 +1,5 @@
		// eslint-disable-next-line @typescript-eslint/no-explicit-any
		const omit = (keysToOmit: string[], originalObj = {}): any =>

Conversation

TheNoodleMoose commented Feb 15, 2021

Uh oh!

vercel Bot commented Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheNoodleMoose commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OzzieOrca commented Feb 16, 2021

Uh oh!

TheNoodleMoose commented Feb 17, 2021

Uh oh!

OzzieOrca left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheNoodleMoose Feb 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

TheNoodleMoose commented Feb 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

OzzieOrca left a comment

Choose a reason for hiding this comment

Uh oh!

TheNoodleMoose commented Feb 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel Bot commented Feb 15, 2021 •

edited

Loading

TheNoodleMoose Feb 19, 2021 •

edited

Loading