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.

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 "experimental" designation #60837

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { DOWN } from '@wordpress/keycodes';
import {
ToolbarButton,
Dropdown,
__experimentalAlignmentMatrixControl as AlignmentMatrixControl,
AlignmentMatrixControl,
} from '@wordpress/components';

const noop = () => {};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,8 @@ export default function FontAppearanceControl( props ) {
);
};

console.debug('selectOptions: ', selectOptions );

return (
hasStylesOrWeights && (
<CustomSelectControl
Expand All @@ -212,8 +214,11 @@ export default function FontAppearanceControl( props ) {
describedBy={ getDescribedBy() }
options={ selectOptions }
value={ currentSelection }
onChange={ ( { selectedItem } ) =>
onChange( selectedItem.style )
onChange={ ( { selectedItem } ) => {
console.debug( selectedItem );
// @todo style is missing from the object
onChange( selectedItem.style );
}
}
/>
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
* WordPress dependencies
*/
import {
__experimentalBorderBoxControl as BorderBoxControl,
__experimentalHasSplitBorders as hasSplitBorders,
__experimentalIsDefinedBorder as isDefinedBorder,
BorderBoxControl,
hasSplitBorders,
isDefinedBorder,
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
__experimentalItemGroup as ItemGroup,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { __ } from '@wordpress/i18n';
import {
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
__experimentalBoxControl as BoxControl,
BoxControl,
__experimentalHStack as HStack,
__experimentalUnitControl as UnitControl,
__experimentalUseCustomUnits as useCustomUnits,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,8 @@ export default function TypographyPanel( {
fontStyle: newFontStyle,
fontWeight: newFontWeight,
} ) => {
console.debug(fontStyle);
console.debug(fontWeight);
onChange( {
...value,
typography: {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/hooks/border.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { hasBlockSupport, getBlockSupport } from '@wordpress/blocks';
import { __experimentalHasSplitBorders as hasSplitBorders } from '@wordpress/components';
import { hasSplitBorders } from '@wordpress/components';
import { Platform, useCallback, useMemo } from '@wordpress/element';
import { addFilter } from '@wordpress/hooks';
import { useSelect } from '@wordpress/data';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,7 @@
/**
* WordPress dependencies
*/
import {
Button,
__experimentalConfirmDialog as ConfirmDialog,
} from '@wordpress/components';
import { Button, ConfirmDialog } from '@wordpress/components';
Copy link
Member

Choose a reason for hiding this comment

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

I think updating all of Gutenberg to use the stable component imports can be its own PR. There are quite a few of them that this PR isn't covering just yet, and splitting to smaller PRs could help with reviewing.

Copy link
Member

Choose a reason for hiding this comment

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

Doing it in a separate PR would also help to check that we didn't remove any __experimental exports by mistake.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! I'll rework that 👍🏻

import { store as coreStore, useEntityId } from '@wordpress/core-data';
import { useDispatch } from '@wordpress/data';
import { useState } from '@wordpress/element';
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/table/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
TextControl,
ToggleControl,
ToolbarDropdownMenu,
__experimentalHasSplitBorders as hasSplitBorders,
hasSplitBorders,
} from '@wordpress/components';
import {
alignLeft,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ AlignmentMatrixControl components enable adjustments to horizontal and vertical

```jsx
import { useState } from 'react';
import { __experimentalAlignmentMatrixControl as AlignmentMatrixControl } from '@wordpress/components';
import { AlignmentMatrixControl } from '@wordpress/components';

const Example = () => {
const [ alignment, setAlignment ] = useState( 'center center' );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import type { AlignmentMatrixControlProps } from './types';
* AlignmentMatrixControl components enable adjustments to horizontal and vertical alignments for UI.
*
* ```jsx
* import { __experimentalAlignmentMatrixControl as AlignmentMatrixControl } from '@wordpress/components';
* import { AlignmentMatrixControl } from '@wordpress/components';
* import { useState } from '@wordpress/element';
*
* const Example = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { HStack } from '../../h-stack';
import type { AlignmentMatrixControlProps } from '../types';

const meta: Meta< typeof AlignmentMatrixControl > = {
title: 'Components (Experimental)/AlignmentMatrixControl',
title: 'Components/AlignmentMatrixControl',
component: AlignmentMatrixControl,
subcomponents: {
// @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ show "Mixed" placeholder text.

```jsx
import { useState } from 'react';
import { __experimentalBorderBoxControl as BorderBoxControl } from '@wordpress/components';
import { BorderBoxControl } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

const colors = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ const UnconnectedBorderBoxControl = (
* view's width input would show "Mixed" placeholder text.
*
* ```jsx
* import { __experimentalBorderBoxControl as BorderBoxControl } from '@wordpress/components';
* import { BorderBoxControl } from '@wordpress/components';
* import { __ } from '@wordpress/i18n';
*
* const colors = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import Button from '../../button';
import { BorderBoxControl } from '../';

const meta: Meta< typeof BorderBoxControl > = {
title: 'Components (Experimental)/BorderBoxControl',
title: 'Components/BorderBoxControl',
component: BorderBoxControl,
argTypes: {
onChange: { action: 'onChange' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ a "shape" abstraction.

```jsx
import { useState } from 'react';
import { __experimentalBorderControl as BorderControl } from '@wordpress/components';
import { BorderControl } from '@wordpress/components';
import { __ } from '@wordpress/i18n';

const colors = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ const UnconnectedBorderControl = (
* a "shape" abstraction.
*
* ```jsx
* import { __experimentalBorderControl as BorderControl } from '@wordpress/components';
* import { BorderControl } from '@wordpress/components';
* import { __ } from '@wordpress/i18n';
*
* const colors = [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { BorderControl } from '..';
import type { Border } from '../types';

const meta: Meta< typeof BorderControl > = {
title: 'Components (Experimental)/BorderControl',
title: 'Components/BorderControl',
component: BorderControl,
argTypes: {
onChange: {
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/box-control/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ BoxControl components let users set values for Top, Right, Bottom, and Left. Thi

```jsx
import { useState } from 'react';
import { __experimentalBoxControl as BoxControl } from '@wordpress/components';
import { BoxControl } from '@wordpress/components';

const Example = () => {
const [ values, setValues ] = useState( {
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/box-control/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ function useUniqueId( idProp?: string ) {
* This can be used as an input control for values like `padding` or `margin`.
*
* ```jsx
* import { __experimentalBoxControl as BoxControl } from '@wordpress/components';
* import { BoxControl } from '@wordpress/components';
* import { useState } from '@wordpress/element';
*
* const Example = () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { useState } from '@wordpress/element';
import BoxControl from '../';

const meta: Meta< typeof BoxControl > = {
title: 'Components (Experimental)/BoxControl',
title: 'Components/BoxControl',
component: BoxControl,
argTypes: {
values: { control: { type: null } },
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/confirm-dialog/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Allows the component to be used standalone, just by declaring it as part of anot
Activating this mode is as simple as omitting the `isOpen` prop. The only mandatory prop, in this case, is the `onConfirm` callback. The message is passed as the `children`. You can pass any JSX you'd like, which allows to further format the message or include sub-component if you'd like:

```jsx
import { __experimentalConfirmDialog as ConfirmDialog } from '@wordpress/components';
import { ConfirmDialog } from '@wordpress/components';

function Example() {
return (
Expand All @@ -44,7 +44,7 @@ Let the parent component control when the dialog is open/closed. It's activated

```jsx
import { useState } from 'react';
import { __experimentalConfirmDialog as ConfirmDialog } from '@wordpress/components';
import { ConfirmDialog } from '@wordpress/components';

function Example() {
const [ isOpen, setIsOpen ] = useState( true );
Expand Down
4 changes: 2 additions & 2 deletions packages/components/src/confirm-dialog/component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ const UnconnectedConfirmDialog = (
* Activating this mode is as simple as omitting the `isOpen` prop. The only mandatory prop, in this case, is the `onConfirm` callback. The message is passed as the `children`. You can pass any JSX you'd like, which allows to further format the message or include sub-component if you'd like:
*
* ```jsx
* import { __experimentalConfirmDialog as ConfirmDialog } from '@wordpress/components';
* import { ConfirmDialog } from '@wordpress/components';
*
* function Example() {
* return (
Expand All @@ -158,7 +158,7 @@ const UnconnectedConfirmDialog = (
* - You'll want to update the state that controls `isOpen` by updating it from the `onCancel` and `onConfirm` callbacks.
*
*```jsx
* import { __experimentalConfirmDialog as ConfirmDialog } from '@wordpress/components';
* import { ConfirmDialog } from '@wordpress/components';
* import { useState } from '@wordpress/element';
*
* function Example() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { ConfirmDialog } from '../component';

const meta: Meta< typeof ConfirmDialog > = {
component: ConfirmDialog,
title: 'Components (Experimental)/ConfirmDialog',
title: 'Components/ConfirmDialog',
argTypes: {
isOpen: {
control: { type: null },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,16 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
// Forward props + store from v2 implementation
const store = Ariakit.useSelectStore( {
async setValue( nextValue ) {
console.debug('nextValue: ', nextValue);
if ( ! onChange ) return;

// Executes the logic in a microtask after the popup is closed.
// This is simply to ensure the isOpen state matches that in Downshift.
await Promise.resolve();
const state = store.getState();

console.debug(state);

const changeObject = {
highlightedIndex: state.renderedItems.findIndex(
( item ) => item.value === nextValue
Expand All @@ -45,7 +48,9 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
isOpen: state.open,
selectedItem: {
name: nextValue as string,
key: nextValue as string,
key: nextValue as string, // @todo Why key and name have the same value?
// @todo this structure is not compatible with the one from FontAppearanceControl, which
// expects an `style` attribute. Probably something to be fixed in FontAppearanceControl? Or am I missing something here?
},
type: '',
};
Expand Down Expand Up @@ -129,3 +134,14 @@ function CustomSelectControl( props: LegacyCustomSelectProps ) {
}

export default CustomSelectControl;

// for backwards compatibility
export function StableCustomSelectControl( props: LegacyCustomSelectProps ) {
console.debug(props);
return (
<CustomSelectControl
{ ...props }
__experimentalShowSelectedHint={ false }
/>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { useState } from '@wordpress/element';
import CustomSelectControlV2 from '..';

const meta: Meta< typeof CustomSelectControlV2 > = {
title: 'Components (Experimental)/CustomSelectControl v2/Default',
title: 'Components/CustomSelectControl v2/Default',
component: CustomSelectControlV2,
subcomponents: {
// @ts-expect-error - See https://github.com/storybookjs/storybook/issues/23170
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import CustomSelectControl from '../legacy-component';
import * as V1Story from '../../custom-select-control/stories/index.story';

const meta: Meta< typeof CustomSelectControl > = {
title: 'Components (Experimental)/CustomSelectControl v2/Legacy',
title: 'Components/CustomSelectControl v2/Legacy',
component: CustomSelectControl,
argTypes: {
onChange: { control: { type: null } },
Expand Down
5 changes: 4 additions & 1 deletion packages/components/src/index.native.js
Copy link
Member

Choose a reason for hiding this comment

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

Let's not touch the *.native.js stuff for now, they are completely separate.

Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ export {

export { default as __experimentalStyleProvider } from './style-provider';
export { default as BaseControl } from './base-control';
export { hasSplitBorders as __experimentalHasSplitBorders } from './border-box-control/utils';
export {
hasSplitBorders as __experimentalHasSplitBorders,
hasSplitBorders,
} from './border-box-control/utils';
export { default as TextareaControl } from './textarea-control';
export { default as PanelBody } from './panel/body';
export { default as PanelActions } from './panel/actions';
Expand Down
24 changes: 20 additions & 4 deletions packages/components/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ export {
} from '@wordpress/primitives';

// Components.
export { default as __experimentalAlignmentMatrixControl } from './alignment-matrix-control';
export {
default as __experimentalAlignmentMatrixControl,
Copy link
Member

Choose a reason for hiding this comment

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

While I appreciate that we're keeping the __experimental exports for backward compatibility reasons, I wonder if we should deprecate __experimental imports now that the components are stable. That will nudge 3rd party developers to migrate to stable imports, otherwise they might be stuck with the __experimental ones forever

Copy link
Member

Choose a reason for hiding this comment

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

There are so many "shades" of deprecation — hard, soft, TS-only, TSDoc only... 😂 For these I think a TSDoc @deprecated Import AlignmentMatrixControl instead would be appropriate. Is that what you had in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, it's odd that deprecation can be so complex 😅

Anyway, imagine I'm a plugin dev that uses some experimental components in my plugin. How will a TSDoc @deprecated help me find out that I'm using a deprecated import and I have to replace it with the canonical stable import? Will I even be notified about it by my editor, or does that rely on me being very attentive to reading the type hints provided by my editor?

Copy link
Member Author

@fullofcaffeine fullofcaffeine Apr 18, 2024

Choose a reason for hiding this comment

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

Will I even be notified about it by my editor, or does that rely on me being very attentive to reading the type hints provided by my editor?

I managed to make the @deprecate annotation work when it's added just before the actual export symbol, inside the export block:

export {
	/**
	 * @deprecated Since version X.X. Will be removed in version X.Y. Use `ConfirmDialog` instead.
	 */
	ConfirmDialog as __experimentalConfirmDialog,
	ConfirmDialog,
} from './confirm-dialog';

I can confirm it doesn't affect the second export there:

Screenshot 2024-04-18 at 12 51 30 p m

And of the @deprecated declaration, it will render like this in vscode (notice the strikethrough in the import and the warning in the IntelliSense popup):

Screenshot 2024-04-18 at 12 48 28 p m Screenshot 2024-04-18 at 12 49 04 p m

So I assume all editors/IDEs that support tsdoc will warn users about it.

If that's a good way to prevent developers from continuing to use it, that is another story. It might be good to do the deprecation gradually? First soft-deprecate using a tsdoc, announce in the changelog and perhaps a Make post, then in another (major?) version, completely remove the __experimental exports.

Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

And yeah, gradual deprecation sounds good to me. We should likely document the plan for that in the related issue, including some releases where we aim to further/hard deprecate.

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, imagine I'm a plugin dev that uses some experimental components in my plugin. How will a TSDoc @deprecated help me find out that I'm using a deprecated import and I have to replace it with the canonical stable import? Will I even be notified about it by my editor, or does that rely on me being very attentive to reading the type hints provided by my editor?

My current stance on this is,

  1. Do we plan to hard deprecate it?
  2. Does switching to the new thing provide the consumer with actual benefits over the old thing?

If the answer is "no" to both questions, we shouldn't log a console warning and we shouldn't block the TS from compiling. We shouldn't demand a dev's attention for what is essentially a housekeeping detail. If they care about housekeeping details, they'll do it.

And to be clear, personally I don't think the __experimental exports are worth hard deprecating. It's virtually free to maintain this BC, compared to the cost on the ecosystem if we yank them.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, the complex deprecation discussion again 😅

I think we have far too many cases that are cheap/free to maintain, but because we have too many, it adds so much noise. I wish we had a clear deprecation policy that actually allows us to get rid of deprecated features at some point.

If I come back to planet Earth though and draw a line on the tradeoffs, you're right and it should be just fine to continue soft-deprecating as suggested.

AlignmentMatrixControl,
} from './alignment-matrix-control';
export {
default as Animate,
getAnimateClassName as __unstableGetAnimateClassName,
Expand All @@ -33,11 +36,20 @@ export {
hasSplitBorders as __experimentalHasSplitBorders,
isDefinedBorder as __experimentalIsDefinedBorder,
isEmptyBorder as __experimentalIsEmptyBorder,
BorderBoxControl,
hasSplitBorders,
isDefinedBorder,
isEmptyBorder,
Comment on lines +40 to +42
Copy link
Member

Choose a reason for hiding this comment

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

Ok, these three exports are sounding major alarm bells for me 😱 Not sure what's going on there. Let's keep these experimental for now so we can investigate and figure out a better way.

Copy link
Contributor

Choose a reason for hiding this comment

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

See: #61135

} from './border-box-control';
export { BorderControl as __experimentalBorderControl } from './border-control';
export {
BorderControl as __experimentalBorderControl,
BorderControl,
} from './border-control';
export {
default as __experimentalBoxControl,
applyValueToSides as __experimentalApplyValueToSides,
default as BoxControl,
applyValueToSides,
} from './box-control';
export { default as Button } from './button';
export { default as ButtonGroup } from './button-group';
Expand All @@ -62,8 +74,12 @@ export {
CompositeItem as __unstableCompositeItem,
useCompositeState as __unstableUseCompositeState,
} from './composite';
export { ConfirmDialog as __experimentalConfirmDialog } from './confirm-dialog';
export { StableCustomSelectControl as CustomSelectControl } from './custom-select-control';
export {
ConfirmDialog as __experimentalConfirmDialog,
ConfirmDialog,
} from './confirm-dialog';
export { StableCustomSelectControl as CustomSelectControl } from './custom-select-control-v2/legacy-component';
export { default as CustomSelectControlV2 } from './custom-select-control-v2';
export { default as Dashicon } from './dashicon';
export { default as DateTimePicker, DatePicker, TimePicker } from './date-time';
export { default as __experimentalDimensionControl } from './dimension-control';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ import styled from '@emotion/styled';
* WordPress dependencies
*/
import {
__experimentalBoxControl as BoxControl,
BoxControl,
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
__experimentalUnitControl as UnitControl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import {
Button,
__experimentalConfirmDialog as ConfirmDialog,
ConfirmDialog,
__experimentalHStack as HStack,
__experimentalHeading as Heading,
__experimentalNavigatorProvider as NavigatorProvider,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { store as coreStore } from '@wordpress/core-data';
import {
PanelBody,
__experimentalVStack as VStack,
__experimentalHasSplitBorders as hasSplitBorders,
hasSplitBorders,
} from '@wordpress/components';
import { __, sprintf } from '@wordpress/i18n';

Expand Down
Loading
Loading