QueryControls: refactor away from lodash.groupBy

[flootr]

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:

• Remove lodash dependency from '@wordpress/element' #16938 (comment)
• Remove lodash dependency from WordPress packages. #17025
• @wordpress/api-fetch package has lodash as a dependency #39495

How?

Replaces _.groupBy with a native .reduce function.

Testing Instructions

1. Verify unit tests still pass
2. 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]

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.

[flootr]

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)

[tyxla]

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

[ciampo]

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

[flootr]

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

[ciampo]

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.

[flootr]

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.

[ciampo]

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

[flootr]

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!

[tyxla]

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

[tyxla]

The shorter version wins 👍

[github-actions]

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]

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]

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]

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.