TypeScript: migrate server-side-render package to TS#71383
TypeScript: migrate server-side-render package to TS#71383kushagra-goyal-14 wants to merge 1 commit intoWordPress:trunkfrom
Conversation
kushagra-goyal-14
requested review from
ajitbohra and
fabiankaegy
as code owners
|
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. |
im3dabasia
added
[Type] Code Quality
manzoorwanijk
left a comment
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks good. I have some suggestions for minor improvements
| } ); | ||
| } ) | ||
| .catch( ( error ) => { | ||
| .catch( ( error: Error ) => { |
There was a problem hiding this comment.
The error is always unknown on runtime and that is how it should be as a type. If it's just have access to error.name, you can add error instanceof Error check before that. Otherwise, you can simply remove this type annotation here.
| /** | ||
| * External dependencies | ||
| */ | ||
| import type { ComponentType, ReactNode } from 'react'; |
There was a problem hiding this comment.
Can we please not import it here? We can directly use React.ComponentType or React.ReactNode without the need for any import
| // translators: %s: error message describing the problem | ||
| __( 'Error loading block: %s' ), | ||
| message | ||
| message || 'Unknown error' |
There was a problem hiding this comment.
This should be translated
| message || 'Unknown error' | |
| message || __( 'Unknown error' ) |
There was a problem hiding this comment.
Another approach would be to get rid of this extra string by not making message optional in ErrorPlaceholderProps and then using message={ error! } in <ErrorResponsePlaceholder />. We can assert that because the description for error mentions "The error message (available when status is 'error').", which is already the case when it's used there.
|
|
||
| export function removeBlockSupportAttributes( attributes ) { | ||
| export function removeBlockSupportAttributes( | ||
| attributes: Record< string, unknown > |
There was a problem hiding this comment.
Instead of making an assertion below for attributes?.style, we can type it here
| attributes: Record< string, unknown > | |
| attributes: Record< string, unknown > & { | |
| style?: Record< string, unknown >; | |
| } |
| typography, | ||
| ...restStyles | ||
| } = attributes?.style || {}; | ||
| } = ( attributes?.style as Record< string, unknown > ) || {}; |
There was a problem hiding this comment.
We can then revert this change
| } = ( attributes?.style as Record< string, unknown > ) || {}; | |
| } = attributes?.style || {}; |
3 participants
What?
Part of #67691
Migrating the server-side-render package to TypeScript.
This PR supersedes #70755 as discussed in this PR
Why?
To enhance DX and add type safety.
How?
By porting the code and tests to TypeScript.
Testing Instructions
Typecheck and unit tests.