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

Fix misc type compilation errors in editor and block editor packages #67410

Merged
merged 2 commits into from
Nov 29, 2024
+11 −7
Merged

Fix misc type compilation errors in editor and block editor packages #67410

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
406936a
I noticed there were some typing errors locally.
ramonjd Nov 29, 2024
527bcb7
I noticed there were some typing errors locally.
ramonjd Nov 29, 2024
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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

- Upgraded `@ariakit/react` (v0.4.13) and `@ariakit/test` (v0.4.5) ([#65907](https://github.com/WordPress/gutenberg/pull/65907)).
- Upgraded `@ariakit/react` (v0.4.15) and `@ariakit/test` (v0.4.7) ([#67404](https://github.com/WordPress/gutenberg/pull/67404)).
- Exported the `WPCompleter` type as it was being used in block-editor/autocompleters ([#67410](https://github.com/WordPress/gutenberg/pull/67410)).

### Bug Fixes

Expand Down
1 change: 1 addition & 0 deletions packages/components/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ export { Heading as __experimentalHeading } from './heading';
export { HStack as __experimentalHStack } from './h-stack';
export { default as Icon } from './icon';
export type { IconType } from './icon';
export type { WPCompleter } from './autocomplete/types.ts';
Copy link
Member Author

Choose a reason for hiding this comment

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

@ciampo

The WPCompleter type is being used in block-editor/autocompleters three times, e.g.,

gutenberg/packages/block-editor/src/autocompleters/link.js

Line 13 in 8e42a74

/** @typedef {import('@wordpress/components').WPCompleter} WPCompleter */

/** @typedef {import('@wordpress/components').WPCompleter} WPCompleter */

I was wondering whether files that import the type should be doing it at all?

In which case the JS Docs in block-editor/autocompleters could be updated and this export removed.

Copy link
Contributor

@ciampo ciampo Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hey Ramon, thank you for the ping! For future instances, using the @WordPress/gutenberg-components group handle may be better in case one of us can't reply straight away (that was me last week 😅 )

In general, we haven't really exported types from the package, so my first instinct would be to revert this change, and then consider the best course of action — which is to probably fix those type imports across Gutenberg

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do, thank you!

Copy link
Member Author

Choose a reason for hiding this comment

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

Revert/update PR:

export { default as IconButton } from './button/deprecated';
export {
ItemGroup as __experimentalItemGroup,
Expand Down
7 changes: 4 additions & 3 deletions packages/editor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1462,11 +1462,11 @@ Wrapper component that renders its children only if the post can trashed.
_Parameters_

- _props_ `Object`: - The component props.
- _props.children_ `React.ReactEl`: - The child components to render.
- _props.children_ `React.ReactNode`: - The child components to render.

_Returns_

- `React.ReactElement`: The rendered child components or null if the post can not trashed.
- `React.ReactNode`: The rendered child components or null if the post can not trashed.

### PostTypeSupportCheck

Expand Down Expand Up @@ -1494,7 +1494,8 @@ _Usage_

_Parameters_

- _onClose_ `Function`: Callback function to be executed when the popover is closed.
- _props_ `{ onClose: () => void }`: The props for the component.
- _props.onClose_ `() => void`: Callback function to be executed when the popover is closed.

_Returns_

Expand Down
6 changes: 3 additions & 3 deletions packages/editor/src/components/post-trash/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ import { GLOBAL_POST_TYPES } from '../../store/constants';
/**
* Wrapper component that renders its children only if the post can trashed.
*
* @param {Object} props - The component props.
* @param {React.ReactEl} props.children - The child components to render.
* @param {Object} props - The component props.
* @param {React.ReactNode} props.children - The child components to render.
Copy link
Member Author

Choose a reason for hiding this comment

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

This was probably a typo. React.ReactEl doesn't exist

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have the whole context around these changes, but do we actually need ReactNode instead of ReactElement ? In other words, do we ever need to render children that are null, undefined, boolean, number, string ?

Just asking in case the right choice here was ReactElement.

If we go for ReactElement, we could also consider updating the return type to ReactElement | null

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure, but I'm sure ReactElement is a good substitute here 👍🏻

I'll do a follow up.

*
* @return {React.ReactElement} The rendered child components or null if the post can not trashed.
* @return {React.ReactNode} The rendered child components or null if the post can not trashed.
*/
export default function PostTrashCheck( { children } ) {
const { canTrashPost } = useSelect( ( select ) => {
Expand Down
3 changes: 2 additions & 1 deletion packages/editor/src/components/post-url/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ import { store as editorStore } from '../../store';
* <PostURL />
* ```
*
* @param {Function} onClose Callback function to be executed when the popover is closed.
* @param {{ onClose: () => void }} props The props for the component.
* @param {() => void} props.onClose Callback function to be executed when the popover is closed.
*
* @return {React.ReactNode} The rendered PostURL component.
*/
Expand Down
Loading