Fix misc type compilation errors in editor and block editor packages #67410
+11
−7
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ramonjd
commented
Nov 29, 2024
| @@ -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'; | |||
There was a problem hiding this comment.
The WPCompleter type is being used in block-editor/autocompleters three times, e.g.,
/** @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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Revert/update PR:
| * @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. |
There was a problem hiding this comment.
This was probably a typo. React.ReactEl doesn't exist
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I'm not sure, but I'm sure ReactElement is a good substitute here 👍🏻
I'll do a follow up.
|
Size Change: 0 B Total Size: 1.83 MB ℹ️ View Unchanged
|
Mamaduka
approved these changes
Nov 29, 2024
ramonjd
force-pushed
the
fix/type-complication-errors
branch
from
a1dafb2 to
527bcb7
Compare
|
Thanks for testing @Mamaduka I'll merge for now. Can always follow up on the |
im3dabasia
pushed a commit
to im3dabasia/gutenberg
that referenced
this pull request
Dec 4, 2024
…ordPress#67410) Co-authored-by: ramonjd <[email protected]> Co-authored-by: Mamaduka <[email protected]>
ramonjd
added a commit
that referenced
this pull request
Dec 4, 2024
… update types to Object for now. Use ReactElement instead of ReactNode
michalczaplinski
pushed a commit
that referenced
this pull request
Dec 5, 2024
…67410) Co-authored-by: ramonjd <[email protected]> Co-authored-by: Mamaduka <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What? Why? How?
I noticed while working on #67406 there were some typing errors locally.
Some of the JS Docs type annotations weren't quite right. Updated.
Also,
WPCompleterwas used in the block editor package for autocompleters, but it wasn't exported. So I exported it YOLO style.Why?
Testing Instructions
Build should pass as expected