Components: remove lodash from `context/getStyledClassName`: by flootr · Pull Request #48688 · WordPress/gutenberg

flootr · 2023-03-02T13:10:36Z

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:

How?

Replaces _.kebabCase with paramCase from the change-case package (already used in the components package).

Testing Instructions

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

tyxla · 2023-03-03T13:56:25Z

packages/components/src/ui/context/get-styled-class-name-from-key.ts

 * External dependencies
 */
-import { kebabCase } from 'lodash';
+import { paramCase as kebabCase } from 'change-case';


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.

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.

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.

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?

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.

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?

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

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?

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.

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 · 2023-03-15T13:50:40Z

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

flootr self-assigned this Mar 2, 2023

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

flootr marked this pull request as ready for review March 3, 2023 10:10

flootr requested a review from ajitbohra as a code owner March 3, 2023 10:10

flootr requested review from ciampo and tyxla March 3, 2023 10:10

tyxla reviewed Mar 3, 2023

View reviewed changes

ciampo requested a review from sarayourfriend March 6, 2023 11:48

tyxla approved these changes Mar 13, 2023

View reviewed changes

ciampo approved these changes Mar 15, 2023

View reviewed changes

flootr mentioned this pull request Mar 15, 2023

contextConnect: cleanup namespace construction #49065

Closed

context/getStyledClassName: refactor away from lodash.kebabCase

d94976c

flootr force-pushed the refactor/ctx-get-styled-cn-lodash branch from 5dd0e3b to d94976c Compare March 15, 2023 13:17

flootr merged commit 0f38d66 into trunk Mar 15, 2023

flootr deleted the refactor/ctx-get-styled-cn-lodash branch March 15, 2023 14:58

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

	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 ) }`,

Conversation

flootr commented Mar 2, 2023

What?

Why?

How?

Testing Instructions

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!

flootr Mar 13, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

ciampo left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

flootr Mar 13, 2023 •

edited

Loading