Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Components: remove lodash from context/getStyledClassName: #48688

Merged
merged 1 commit into from
Mar 15, 2023
Merged

Components: remove lodash from context/getStyledClassName: #48688

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/components/src/ui/context/get-styled-class-name-from-key.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { kebabCase } from 'lodash';
import { paramCase as kebabCase } from 'change-case';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@flootr flootr Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😁

import memoize from 'memize';

/**
Expand Down