Components: remove lodash from context/getStyledClassName:

[flootr]

What?

This PR removes Lodash's _.kebabCase() from the getStyledClassName function used in ui/context.

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 _.kebabCase with paramCase from the change-case package (already used in the components package).

Testing Instructions

1. Verify unit tests still pass; this change should be covered there
2. Start storybook locally and verify the story for the ContextSystemProvider works and looks as expected (compare with trunk here: ContextSystemProvider)

[tyxla]

I may be wrong here, but I think some incorrect types might be deceiving us here.

See this usage of getStyledClassNameFromKey():

gutenberg/packages/components/src/ui/context/context-connect.ts

Lines 94 to 106 in 8dee3e6

	if ( Array.isArray( namespace ) ) {
	mergedNamespace = [ ...mergedNamespace, ...namespace ];
	}
	if ( typeof namespace === 'string' ) {
	mergedNamespace = [ ...mergedNamespace, namespace ];
	}
	// @ts-expect-error We can't rely on inferred types here because of the
	// `as` prop polymorphism we're handling in https://github.com/WordPress/gutenberg/blob/9620bae6fef4fde7cc2b7833f416e240207cda29/packages/components/src/ui/context/wordpress-component.ts#L32-L33
	return Object.assign( WrappedComponent, {
	[ CONNECT_STATIC_NAMESPACE ]: [ ...new Set( mergedNamespace ) ],
	displayName: namespace,
	selector: `.${ getStyledClassNameFromKey( namespace ) }`,

Seems like namespace could be an array? If that happens, this will crash, while _.kebabCase( [ 'test', 'x' ] ) will work just fine, returning test-x.

This other use of getStyledClassNameFromKey() is in a similar situation like the one I reported here:

gutenberg/packages/components/src/ui/context/use-context-system.js

Line 54 in cb10bcf

getStyledClassNameFromKey( namespace ),

Since the namespace is declared by externally exposed API (useContextSystem()), theoretically, the namespace could be test1 and then one of them would yield a test1 class, and the other would yield a test-1 class. That would basically be an unexpected breaking change.

I think to get around this, we need to make sure that the replacement function behaves exactly the same way when mixing letters, numbers and special characters.

[flootr]

Thanks Marin! I'm currently working through your feedback. Regarding

Seems like namespace could be an array?

namespace is typed as string so if you try to pass an array to contextConnect you'd get a type error. I'm not exactly sure why the guard is still in place, it's probably a leftover from #31099 where the Array<string>|string type was replaced with string. My assumption is we could remove it.

[tyxla]

Sounds good for the array case. I'd suggest we remove it then, so we're intentional with our type approach here. Can be done in a separate PR, but I think it should be done before this one lands.

What do you think about this other difference that I mentioned? Reposting here for clarity:

This other use of getStyledClassNameFromKey() is in a similar situation like the one I reported here:

gutenberg/packages/components/src/ui/context/use-context-system.js

Line 54 in cb10bcf

getStyledClassNameFromKey( namespace ),

Since the namespace is declared by externally exposed API (useContextSystem()), theoretically, the namespace could be test1 and then one of them would yield a test1 class, and the other would yield a test-1 class. That would basically be an unexpected breaking change.

I think to get around this, we need to make sure that the replacement function behaves exactly the same way when mixing letters, numbers and special characters.

[ciampo]

Since the namespace is declared by externally exposed API (useContextSystem())

I had a quick look and it doesn't seem that neither getStyledClassNameFromKey nor useContextSystem() are exported by the @wordpress/components package (via the packages/components/src/index.js file).

Maybe @sarayourfriend could help us to clarify any remaining doubts on this, since she worked extensively on the context system?

[tyxla]

I had a quick look and it doesn't seem that neither getStyledClassNameFromKey nor useContextSystem() are exported by the @wordpress/components package (via the packages/components/src/index.js file).

I've manually audited the existing usages and I have no further concern. The use case I've mentioned does not occur, and it won't cause any regressions, so we should be safe to move forward.

The only suggestion I'd have before shipping this one would be removing the unnecessary array handling that we discussed above.

[flootr]

I started looking into removing the array handling we talked about and found a few more things which might be worth addressing. I'm going to discuss those with @ciampo this week (as I don't fully understand a few parts of the components context system). Could we unblock this PR and make your suggestion a follow up @tyxla?

[tyxla]

Cool, a follow-up sounds good 👍 Thanks @flootr

[sarayourfriend]

Maybe @sarayourfriend could help us to clarify any remaining doubts on this, since she worked extensively on the context system?

Happy to help if I can, but can you clarify what the question is?

[tyxla]

It's mostly about the naming of contexts in useContextSystem() and specifically, treating numbers. It was about whether if we call useContextSystem( 'test-1' ), we should expect to get a className of test-1 or test1.

I realize this might be coming from the original g2 implementation, in which case @sarayourfriend might not have the answer too.

I'm fine with making a decision here though, since useContextSystem() is still internal and uses no numbers in context names just yet. That's why I approved the PR.

[sarayourfriend]

I realize this might be coming from the original g2 implementation, in which case @sarayourfriend might not have the answer too.

Yup, I have no idea what the original intention is.

since useContextSystem() is still internal and uses no numbers in context names just yet

Excellent 😁

[tyxla]

LGTM 🚀

[ciampo]

I started looking into removing the array handling we talked about and found a few more things which might be worth addressing. I'm going to discuss those with @ciampo this week (as I don't fully understand a few parts of the components context system).

After discussing with Markus, we decided to keep the changes in this as lean as possible, in order to unblock further work on the lodash migration.

Markus may open a draft PR with some of his WIP about simplifying the Context System, but we won't realistically get back at it before completing the lodash and the TypeScript migration. After that, we will take a moment to assess the Context APIs, and decide the next steps (are these APIs really useful? Should we therefore use them more, or should we instead remove them entirely? If we keep them, what code optimisations can we apply?)

[github-actions]

Flaky tests detected in d94976c.
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/4426740940
📝 Reported issues:

• [Flaky Test] allows shift tabbing to the block toolbar from the first block #49025 in /test/e2e/specs/site-editor/writing-flow.spec.js
• [Flaky Test] doesn't throw a block error when clicking on a link #47747 in /test/e2e/specs/site-editor/template-part.spec.js