`QueryControls`: refactor away from `lodash.groupBy` #48779

flootr · 2023-03-06T12:30:19Z

What?

This PR removes Lodash's _.groupBy() from the buildTermsTree function used in QueryControls

Why?

Lodash is known to unnecessarily inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality. See these for more information and rationale:

How?

Replaces _.groupBy with a native .reduce function.

Testing Instructions

Verify unit tests still pass
Verify stories for QueryControls still work as expected

tyxla

Looks good to me 👍

There's a couple of suggestions to handle before merging this, but I think it looks great and ready to ship afterwards 🚀

tyxla · 2023-03-07T10:32:33Z

packages/components/src/query-controls/terms.ts

-		'parent'
-	);
-	if ( termsByParent.null && termsByParent.null.length ) {
+	if ( ! ensureParents( flatTermsWithParentAndChildren ) ) {


I wonder if we could just inline ensureParents(). For me it was easier to read this line:

terms.every( ( term ) => term.parent !== null )

than having to read what ensureParents() does. It's not immediately clear what the function does too - does it add a parent to each term? Or does it filter only terms with parents? I wouldn't expect from a function with this name to return true only when ALL passed terms have parents. Inlining the condition will solve this mystery IMHO.

Inlining didn't work for me because TypeScript doesn't infer that parent isn't null anymore. That's why I created a type guard. That's the important part:

terms is ( TermWithParentAndChildren & { parent: number } )[] (L13)

I see. Let's think of a name that matches the purpose better then.

I have an idea — what if we tweaked the way the termsByParent dictionary is created, by literally using the 'null' string as a fallback key when the parent is actually null ?

Something like this

diff --git a/packages/components/src/query-controls/terms.ts b/packages/components/src/query-controls/terms.ts index 951ca626be..9f5688fc91 100644 --- a/packages/components/src/query-controls/terms.ts +++ b/packages/components/src/query-controls/terms.ts @@ -8,11 +8,6 @@ import type { TermsByParent, } from './types'; -const ensureParents = ( - terms: TermWithParentAndChildren[] -): terms is ( TermWithParentAndChildren & { parent: number } )[] => { - return terms.every( ( term ) => term.parent !== null ); -}; /** * Returns terms in a tree form. * @@ -29,22 +24,23 @@ export function buildTermsTree( flatTerms: readonly ( Author | Category )[] ) { id: String( term.id ), } ) ); - if ( ! ensureParents( flatTermsWithParentAndChildren ) ) { - return flatTermsWithParentAndChildren; - } - const termsByParent = flatTermsWithParentAndChildren.reduce( ( acc: TermsByParent, term ) => { const { parent } = term; - if ( ! acc[ parent ] ) { - acc[ parent ] = []; + const parentKey = parent ?? 'null'; + if ( ! acc[ parentKey ] ) { + acc[ parentKey ] = []; } - acc[ parent ].push( term ); + acc[ parentKey ].push( term ); return acc; }, {} ); + if ( termsByParent.null && termsByParent.null.length ) { + return flatTermsWithParentAndChildren; + } + const fillWithChildren = ( terms: TermWithParentAndChildren[] ): TermWithParentAndChildren[] => {

We could even decide to use another string instead of 'null' — there shouldn't be any risk of conflict with other dictionary keys, sinceparent should be a number

I think that's essentially how it was solved before. I'll have another look.

I think that's essentially how it was solved before.

Yup, it looks like it. We could keep the same logic, but make it more explicit with a better key name than 'null' ? It could be a quick way to refactor this PR while introducing the least amount of runtime changes possible.

I do think that's a good use case for .every (or .some) and drop out early before constructing the object and then throwing it away (in case). I'll have another look and see what I come up with.

Sounds good. It looks like the buildTermsTree already has some unit tests, so we should have good confidence in applying the changes you suggested!

In a3745b0 I tried to find a better name for the type guard and added a comment about why we're using it. I think that should address the original concern. Please let me know if there's any other feedback!

I'd name the guard function areAllParentsDefined or allTermsHaveParents but then I'd leave it to your judgment.

tyxla · 2023-03-07T10:32:59Z

packages/components/src/query-controls/terms.ts

+		flatTerms.map( ( term ) => ( {
+			children: [],
+			parent: null,
+			...term,
+			id: String( term.id ),
+		} ) );


The shorter version wins 👍

packages/components/src/query-controls/terms.ts

github-actions · 2023-03-13T10:38:54Z

Flaky tests detected in 9f9e3c8.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4403573166
📝 Reported issues:

[Flaky Test] shows changes in a template when a template part it contains is modified #42139 in /test/e2e/specs/site-editor/template-part.spec.js

tyxla

Looks good from my perspective 👍

cc @ciampo for a final review since he also had some prior feedback.

tyxla · 2023-03-13T11:12:12Z

packages/components/CHANGELOG.md

@@ -23,6 +23,7 @@
 -   `navigateRegions` HOC: Convert to TypeScript ([#48632](https://github.com/WordPress/gutenberg/pull/48632)).
 -   `withSpokenMessages`: HOC: Convert to TypeScript ([#48163](https://github.com/WordPress/gutenberg/pull/48163)).
 -   `DimensionControl(Experimental)`: Convert to TypeScript ([#47351](https://github.com/WordPress/gutenberg/pull/47351)).
+-   `QueryControls`: Refactor away from Lodash (`.groupBy`) ([#48779](https://github.com/WordPress/gutenberg/pull/48779))


Nit:

Suggested change

- `QueryControls`: Refactor away from Lodash (`.groupBy`) ([#48779](https://github.com/WordPress/gutenberg/pull/48779))

- `QueryControls`: Refactor away from Lodash (`.groupBy`) ([#48779](https://github.com/WordPress/gutenberg/pull/48779)).

tyxla · 2023-03-13T11:14:08Z

packages/components/src/query-controls/terms.ts

-		'parent'
-	);
-	if ( termsByParent.null && termsByParent.null.length ) {
+	if ( ! ensureParents( flatTermsWithParentAndChildren ) ) {


I'd name the guard function areAllParentsDefined or allTermsHaveParents but then I'd leave it to your judgment.

ciampo

cc @ciampo for a final review since he also had some prior feedback.

🚀

Thank you for the extra-effort in improving the readability of the code!

tyxla · 2023-03-21T10:46:22Z

I've just opened a very similar PR in the editor package: #49224.

Those should likely come from the same place, but it was like that before, so this is definitely work for another PR.

flootr added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Mar 6, 2023

flootr self-assigned this Mar 6, 2023

flootr marked this pull request as ready for review March 6, 2023 12:47

flootr requested a review from ajitbohra as a code owner March 6, 2023 12:47

flootr requested review from tyxla, mirka, ciampo, brookewp and chad1008 March 6, 2023 12:47

tyxla approved these changes Mar 7, 2023

View reviewed changes

flootr force-pushed the refactor/query-controls-lodash branch from f979041 to a3745b0 Compare March 13, 2023 09:52

tyxla approved these changes Mar 13, 2023

View reviewed changes

ciampo added this to In progress (owned) ⏳ in WordPress Components via automation Mar 14, 2023

ciampo approved these changes Mar 14, 2023

View reviewed changes

flootr added 4 commits March 14, 2023 13:47

QueryControls: refactor away from lodash.groupBy

0c706e3

clean up a little

6c4908f

clarify usage of custom type guard

a9185f9

add changelog entry

58ea300

flootr force-pushed the refactor/query-controls-lodash branch from 9f9e3c8 to 58ea300 Compare March 14, 2023 12:48

flootr merged commit 0a9b980 into trunk Mar 14, 2023

WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Mar 14, 2023

flootr deleted the refactor/query-controls-lodash branch March 14, 2023 14:07

github-actions bot added this to the Gutenberg 15.4 milestone Mar 14, 2023

tyxla mentioned this pull request Mar 21, 2023

Lodash: Remove _.groupBy() from buildTermsTree() in editor #49224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`QueryControls`: refactor away from `lodash.groupBy` #48779

`QueryControls`: refactor away from `lodash.groupBy` #48779

flootr commented Mar 6, 2023

tyxla left a comment

tyxla Mar 7, 2023

flootr Mar 7, 2023

tyxla Mar 7, 2023

ciampo Mar 7, 2023 •

edited

Loading

flootr Mar 7, 2023

ciampo Mar 7, 2023 •

edited

Loading

flootr Mar 8, 2023 •

edited

Loading

ciampo Mar 8, 2023

flootr Mar 13, 2023 •

edited

Loading

tyxla Mar 13, 2023

tyxla Mar 7, 2023

github-actions bot commented Mar 13, 2023

tyxla left a comment

tyxla Mar 13, 2023

tyxla Mar 13, 2023

ciampo left a comment

tyxla commented Mar 21, 2023

	- `QueryControls`: Refactor away from Lodash (`.groupBy`) ([#48779](https://github.com/WordPress/gutenberg/pull/48779))
	- `QueryControls`: Refactor away from Lodash (`.groupBy`) ([#48779](https://github.com/WordPress/gutenberg/pull/48779)).

QueryControls: refactor away from lodash.groupBy #48779

QueryControls: refactor away from lodash.groupBy #48779

Conversation

flootr commented Mar 6, 2023

What?

Why?

How?

Testing Instructions

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

flootr Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flootr Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 13, 2023

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ciampo left a comment

Choose a reason for hiding this comment

tyxla commented Mar 21, 2023

`QueryControls`: refactor away from `lodash.groupBy` #48779

`QueryControls`: refactor away from `lodash.groupBy` #48779

ciampo Mar 7, 2023 •

edited

Loading

ciampo Mar 7, 2023 •

edited

Loading

flootr Mar 8, 2023 •

edited

Loading

flootr Mar 13, 2023 •

edited

Loading