CustomSelectControlV2: Remove legacy adapter layer #59420

mirka · 2024-02-27T20:34:42Z

What?

Removes the legacy adapter layer for CustomSelectControlV2 that was allowing a single component export to be used for both the v1 and v2 prop usages. Instead, we will ship this as two separate components (CustomSelectControl and CustomSelectControlV2).

Why?

After many discussions with @ciampo, we came to the conclusion that it would be better to export as two separate components if a v2 changes the prop API significantly, even if it is technically possible to keep it a single export.

Although it seems clean on the surface, there are some practical disadvantages to keeping it a single export:

In consumer code, it will be very unclear which API version a particular component instance is using. In the worst case, a developer will have to go through docs for every API version to find the correct documentation. It would be much easier if the version number was included in the component name.
Having a single component with the two prop APIs as a union type will complicate TypeScript processing. And although we can present these two APIs in the Storybook docs as separate, we cannot do so in IDE IntelliSense, so it will be more confusing for consumers who rely on the information in the IDE. It will also be a hassle for consumers to create a custom component based on such a component, because the legacy prop types cannot be easily omitted when inheriting types (with React.ComponentProps< typeof CustomSelectControl >).
The code for the unused version cannot be tree-shaken.

Testing Instructions

✅ The package still builds
✅ Tests still pass

github-actions · 2024-02-27T20:51:14Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

mirka · 2024-02-27T20:38:57Z

packages/components/src/custom-select-control-v2/stories/legacy.story.tsx

The Storybook for the legacy usage is also simplified, because we don't need to use separate components/types for extracting the legacy prop API types.

mirka · 2024-02-27T20:41:30Z

packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx

No changes in the tests themselves, just split the files between v1 and v2 API usage.

mirka · 2024-02-27T20:52:48Z

packages/components/src/custom-select-control-v2/test/index.tsx

-describe( 'static typing', () => {
-	<>
-		{ /* @ts-expect-error - when `options` prop is passed, `onChange` should have legacy signature */ }
-		<UncontrolledCustomSelect
-			label="foo"
-			options={ [] }
-			onChange={ ( _: string | string[] ) => undefined }
-		/>
-		<UncontrolledCustomSelect
-			label="foo"
-			options={ [] }
-			onChange={ ( _: { selectedItem: unknown } ) => undefined }
-		/>
-		<UncontrolledCustomSelect
-			label="foo"
-			onChange={ ( _: string | string[] ) => undefined }
-		>
-			foobar
-		</UncontrolledCustomSelect>
-		{ /* @ts-expect-error - when `children` are passed, `onChange` should have new default signature */ }
-		<UncontrolledCustomSelect
-			label="foo"
-			onChange={ ( _: { selectedItem: unknown } ) => undefined }
-		>
-			foobar
-		</UncontrolledCustomSelect>
-	</>;
-} );


I removed these static types tests altogether, because they were only necessary for verifying the complex union type behavior in the legacy adapter layer.

Now that the types are straightforward, we don't need to test them.

Similar to above, it makes sense to remove the With Default Props test group now that we test only the v2 props anyway.

mirka · 2024-02-27T20:56:17Z

packages/components/src/custom-select-control-v2/types.ts

@@ -92,7 +92,6 @@ type LegacyOnChangeObject = {
 };

 export type LegacyCustomSelectProps = {
-	children?: never;


Removed this, since it was only necessary to make TypeScript pick the correct API version from the union type in the legacy adapter layer. (This was the mutually exclusive key.)

aaronrobertshaw

Thanks for putting this PR together 👍

The justification for removing the legacy adapter makes sense.

This PR tests as advertised for me also.

✅ Package builds, no typing errors
✅ Unit tests pass
✅ Code changes make sense (one naive question to follow)
✅ Smoke tested on some PRs that already use the v2 component

Please excuse my ignorance and lack of context & understanding here. What's the benefit in keeping the legacy component within the v2 component directory if it is only to be used for v1 props?

Is it solely so consumers of the old v1 component, only have to switch their import to the v2 legacy component in the hope of getting to a point we can deprecate the v1 component quicker? Does the legacy component within v2 only increase what we have to maintain long term?

tyxla

Looks good, tests well! ✅

Thanks for making a decision in favor of clarity and predictability!

Left a couple of minor test-related comments, but other than that this feels ready to go 🚀

tyxla · 2024-02-28T11:13:02Z

packages/components/src/custom-select-control-v2/legacy-component/test/index.tsx

+	);
+};
+
+describe( 'With Legacy Props', () => {


The legacy component will now always receive legacy props, right? Might make sense to rename this test grouping, or to just remove it altogether.

Good catch, fixed 👍

tyxla · 2024-02-28T11:15:01Z

packages/components/src/custom-select-control-v2/test/index.tsx

-describe( 'static typing', () => {
-	<>
-		{ /* @ts-expect-error - when `options` prop is passed, `onChange` should have legacy signature */ }
-		<UncontrolledCustomSelect
-			label="foo"
-			options={ [] }
-			onChange={ ( _: string | string[] ) => undefined }
-		/>
-		<UncontrolledCustomSelect
-			label="foo"
-			options={ [] }
-			onChange={ ( _: { selectedItem: unknown } ) => undefined }
-		/>
-		<UncontrolledCustomSelect
-			label="foo"
-			onChange={ ( _: string | string[] ) => undefined }
-		>
-			foobar
-		</UncontrolledCustomSelect>
-		{ /* @ts-expect-error - when `children` are passed, `onChange` should have new default signature */ }
-		<UncontrolledCustomSelect
-			label="foo"
-			onChange={ ( _: { selectedItem: unknown } ) => undefined }
-		>
-			foobar
-		</UncontrolledCustomSelect>
-	</>;
-} );


Similar to above, it makes sense to remove the With Default Props test group now that we test only the v2 props anyway.

mirka · 2024-02-28T13:27:36Z

What's the benefit in keeping the legacy component within the v2 component directory if it is only to be used for v1 props?

@aaronrobertshaw Good question. The file structure and naming is kind of temporary while we iterate on it as a WIP. Once I get a few more things brushed up, the plan is to (silently) replace the existing v1 implementation with the one in this legacy-component folder. In theory, consumers of the existing v1 shouldn't even notice that the implementation changed.

Mamaduka · 2024-03-22T10:34:05Z

I noticed that legacy component unit tests are often failing on CI - https://github.com/WordPress/gutenberg/actions/runs/8388712884/job/22973471121#step:6:1185

Screenshot

mirka · 2024-03-22T15:58:53Z

@Mamaduka I've also been noticing a high failure rate on that test. Will look into it!

mirka · 2024-03-22T18:51:04Z

Test flakiness fix proposed in #60133

CustomSelectControlV2: Remove legacy adapter layer

00ce8ad

mirka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Feb 27, 2024

mirka self-assigned this Feb 27, 2024

mirka requested a review from ajitbohra as a code owner February 27, 2024 20:34

Remove never type for children prop on legacy

3ca4776

mirka commented Feb 27, 2024

View reviewed changes

Add changelog

b2c9aae

mirka requested a review from a team February 27, 2024 21:08

mirka mentioned this pull request Feb 27, 2024

CustomSelectControl: refactor with ariakit #55023

Open

85 tasks

aaronrobertshaw approved these changes Feb 28, 2024

View reviewed changes

tyxla approved these changes Feb 28, 2024

View reviewed changes

Remove unnecessary describe wrapper in tests

8828d19

mirka enabled auto-merge (squash) February 28, 2024 13:28

mirka merged commit 31bd66c into trunk Feb 28, 2024
58 of 59 checks passed

mirka deleted the custom-select-remove-wrapper branch February 28, 2024 14:02

github-actions bot added this to the Gutenberg 17.9 milestone Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CustomSelectControlV2: Remove legacy adapter layer #59420

CustomSelectControlV2: Remove legacy adapter layer #59420

mirka commented Feb 27, 2024 •

edited

Loading

github-actions bot commented Feb 27, 2024 •

edited

Loading

mirka Feb 27, 2024

mirka Feb 27, 2024

mirka Feb 27, 2024

tyxla Feb 28, 2024

mirka Feb 27, 2024

aaronrobertshaw left a comment

tyxla left a comment

tyxla Feb 28, 2024

mirka Feb 28, 2024

tyxla Feb 28, 2024

mirka commented Feb 28, 2024

Mamaduka commented Mar 22, 2024

mirka commented Mar 22, 2024

mirka commented Mar 22, 2024

CustomSelectControlV2: Remove legacy adapter layer #59420

CustomSelectControlV2: Remove legacy adapter layer #59420

Conversation

mirka commented Feb 27, 2024 • edited Loading

What?

Why?

Testing Instructions

github-actions bot commented Feb 27, 2024 • edited Loading

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

Choose a reason for hiding this comment

aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

mirka commented Feb 28, 2024

Mamaduka commented Mar 22, 2024

Screenshot

mirka commented Mar 22, 2024

mirka commented Mar 22, 2024

mirka commented Feb 27, 2024 •

edited

Loading

github-actions bot commented Feb 27, 2024 •

edited

Loading