Repository navigation
TypeScript: Migrate packages/list-reusable-blocks package to TypeScript - #70518
manzoorwanijk merged 14 commits into
Conversation
|
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. |
packages/list-reusable-blocks package to TypeScriptpackages/list-reusable-blocks package to TypeScript
|
Can we update this branch from trunk and then we can review it? |
There was a problem hiding this comment.
Pull request overview
This PR migrates the packages/list-reusable-blocks package from JavaScript to TypeScript, adding type annotations and creating type definitions to improve type safety.
Changes:
- Added TypeScript configuration files for the package and root project
- Created type definitions for components and data structures
- Migrated all source files (.js to .tsx/.ts) with type annotations
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Added reference to the list-reusable-blocks package |
| packages/list-reusable-blocks/tsconfig.json | Created TypeScript configuration for the package |
| packages/list-reusable-blocks/src/utils/types.ts | Added type definitions for components and data structures |
| packages/list-reusable-blocks/src/utils/import.ts | Migrated to TypeScript with type annotations |
| packages/list-reusable-blocks/src/utils/file.ts | Migrated to TypeScript with type annotations |
| packages/list-reusable-blocks/src/utils/export.ts | Migrated to TypeScript with type annotations |
| packages/list-reusable-blocks/src/index.tsx | Migrated to TypeScript with proper event typing |
| packages/list-reusable-blocks/src/components/import-form/index.tsx | Migrated to TypeScript with React event types |
| packages/list-reusable-blocks/src/components/import-dropdown/index.tsx | Migrated to TypeScript with proper prop typing |
Comments suppressed due to low confidence (2)
packages/list-reusable-blocks/src/components/import-form/index.tsx:35
- Using
anytype for the parameter defeats the purpose of TypeScript migration. The parameter should be typed asReusableBlockbased on the return type ofimportReusableBlock.
packages/list-reusable-blocks/src/components/import-form/index.tsx:43 - Using
anytype for the errors parameter defeats the purpose of TypeScript migration. Consider defining a proper error type or usingErrortype.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| raw: string; | ||
| }; | ||
| wp_pattern_sync_status: string; | ||
| [ key: string ]: any; |
There was a problem hiding this comment.
Using index signatures [ key: string ]: any weakens type safety. Consider defining specific properties or using a more restrictive approach if these extra properties are truly needed.
| title: string; | ||
| content: string; | ||
| syncStatus?: string; | ||
| [ key: string ]: any; |
There was a problem hiding this comment.
Using index signatures [ key: string ]: any weakens type safety. Consider defining specific properties or using a more restrictive approach if these extra properties are truly needed.
| [ key: string ]: any; | |
| [ key: string ]: unknown; |
| rendered: string; | ||
| }; | ||
| status: string; | ||
| [ key: string ]: any; |
There was a problem hiding this comment.
Using index signatures [ key: string ]: any weakens type safety. Consider defining specific properties or using a more restrictive approach if these extra properties are truly needed.
| [ key: string ]: any; | |
| [ key: string ]: unknown; |
| onUpload: ( data: any ) => void; | ||
| } | ||
|
|
||
| interface ImportFormProps { | ||
| instanceId: string | number; | ||
| onUpload: ( reusableBlock: any ) => void; |
There was a problem hiding this comment.
The type definitions use any type which defeats the purpose of TypeScript migration. The onUpload callback should have a properly typed parameter instead of any.
| onUpload: ( data: any ) => void; | |
| } | |
| interface ImportFormProps { | |
| instanceId: string | number; | |
| onUpload: ( reusableBlock: any ) => void; | |
| onUpload: ( data: ReusableBlockData ) => void; | |
| } | |
| interface ImportFormProps { | |
| instanceId: string | number; | |
| onUpload: ( reusableBlock: ReusableBlockData ) => void; |
| onUpload: ( data: any ) => void; | ||
| } | ||
|
|
||
| interface ImportFormProps { | ||
| instanceId: string | number; | ||
| onUpload: ( reusableBlock: any ) => void; |
There was a problem hiding this comment.
The type definitions use any type which defeats the purpose of TypeScript migration. The onUpload callback should have a properly typed parameter instead of any.
| onUpload: ( data: any ) => void; | |
| } | |
| interface ImportFormProps { | |
| instanceId: string | number; | |
| onUpload: ( reusableBlock: any ) => void; | |
| onUpload: ( data: ReusableBlockData ) => void; | |
| } | |
| interface ImportFormProps { | |
| instanceId: string | number; | |
| onUpload: ( reusableBlock: ReusableBlockData ) => void; |
|
|
||
| interface PostType { | ||
| rest_base: string; | ||
| [ key: string ]: any; |
There was a problem hiding this comment.
Using index signatures [ key: string ]: any weakens type safety. Consider defining specific properties or using a more restrictive approach if these extra properties are truly needed.
| parsedContent = JSON.parse( fileContent ); | ||
| } catch { | ||
| parsedContent = JSON.parse( fileContent ) as ParsedContent; | ||
| } catch ( e ) { |
There was a problem hiding this comment.
| } catch ( e ) { | |
| } catch { |
left a comment
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (5)
packages/list-reusable-blocks/src/utils/file.ts:13
readTextFilenever rejects and doesn’t handleFileReader.onerror/abort. If the read fails (orreader.resultends upnull), the returned promise can hang or resolve to a non-string value (masked by theas stringcast), which can later crash JSON parsing/validation. Consider wiringonerror/onaborttoreject, and only resolving whentypeof reader.result === 'string'(otherwise reject with a clear error).
packages/list-reusable-blocks/src/utils/import.ts:36JSON.parsecan legally return non-object values (e.g.null, a string, number, array). SinceparsedContentis cast toParsedContent, the subsequentparsedContent.__fileaccess can throw a TypeError for inputs like"null". Add a runtime guard thatparsedContentis a non-null object (and ideally not an array) before accessing properties, and treat invalid shapes as an “Invalid pattern JSON file” error.
packages/list-reusable-blocks/src/utils/import.ts:41- The
syncStatusvalidation uses a truthiness check (parsedContent.syncStatus && ...), which allows invalid non-string values like0,null, or''(empty string skips the type check) to bypass validation. Use an explicit undefined check (e.g.parsedContent.syncStatus !== undefined) when validating the type.
packages/list-reusable-blocks/src/components/import-form/index.tsx:49 importReusableBlockis already typed to resolveReusableBlock, but the.thencallback types it asanyand the.catchparameter is alsoany. This defeats the TypeScript migration and makeserrors.messageunsafe. Prefer keeping the inferredReusableBlocktype in.then, and type the caught value asunknownand narrow toError(or a custom shape) before reading.message.
packages/list-reusable-blocks/src/index.tsx:26- The click handler assumes
event.targetis anHTMLElementand immediately readsclassList/dataset.MouseEvent.targetcan be a non-HTMLElement (or evennull), which would throw at runtime despite the cast. Add aninstanceof HTMLElement(orElement) guard before accessingclassList/dataset, and consider validatingNumber(blockId)is finite before callingexportReusableBlockto avoid/NaNrequests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
commented
Apr 7, 2026
|
Hi @manzoorwanijk, Thanks for the clarification and for pointing that out! I’ll revisit this and switch to using the existing Post and PostType types from core-data, instead of redefining them. Could you please take another look and re-review the PR when you get a chance? Thanks again for the helpful feedback |
commented
Apr 8, 2026
|
Hi @manzoorwanijk, |
left a comment
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
packages/list-reusable-blocks/src/utils/file.ts:13
readTextFilecurrently never rejects and will resolvereader.resultwith aas stringcast even thoughFileReader.resultcan benull(or non-string on other read modes). This can leave callers hanging on read errors/abort and can propagate an invalidnullvalue. Consider wiringreader.onerror/reader.onabortto reject, and inonloadverifytypeof reader.result === 'string'before resolving (otherwise reject with an error).
packages/list-reusable-blocks/src/index.tsx:18- The click handler casts
event.targettoHTMLElementand callsclassList/datasetwithout a runtime guard. If the click originates from a non-HTMLElement target, this will throw. Prefer checkingevent.target instanceof HTMLElement(or usingevent.target as Element+'classList' in target) before accessingclassList/dataset.
packages/list-reusable-blocks/src/index.tsx:26 Number( blockId )can produceNaN(or0for an empty string), which then gets interpolated into the REST path inexportReusableBlock(e.g./.../NaN?context=edit). Parse and validate the id before callingexportReusableBlock(e.g.const id = Number.parseInt(blockId, 10)and guard withNumber.isFinite(id)).
packages/list-reusable-blocks/src/utils/import.ts:36- After
JSON.parse, the result can benull, an array, or a primitive. The validation block accessesparsedContent.__filebefore ensuring it’s a non-null object, which can throw a runtime error (e.g. importing a JSON file containingnull). Consider parsing intounknownfirst and adding antypeof parsed === 'object' && parsed !== null && !Array.isArray(parsed)guard before reading properties.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
commented
Apr 13, 2026
|
Thank you for pointing out the missing fields in the package.json. I really appreciate your attention. I’ve made the necessary updates to the PR. Whenever you have a moment, could you please take another look and share your feedback? |
left a comment
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 11 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
packages/list-reusable-blocks/src/utils/import.ts:42
JSON.parse()can return non-object values (e.g.null, arrays, numbers). In those cases, the subsequent property access (parsedContent.__file, etc.) will throw a runtime TypeError instead of the intended "Invalid pattern JSON file" error. Consider parsing tounknownand validating that the result is a non-null plain object with string__file/title/contentbefore accessing properties, so invalid-but-well-formed JSON is handled gracefully.
packages/list-reusable-blocks/src/utils/file.ts:14readTextFile()never rejects: ifFileReadererrors/aborts the Promise will remain pending, andreader.resultis cast tostringeven though it can benull. Addreader.onerror/reader.onaborthandling to reject the Promise (or resolve to a safe default) and ensure the resolved value is actually a string before returning it.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
What?
Part of: #67691
Migrating the
packages/list-reusable-blockspackage to TypeScript.Why?
Type safety.
Testing Instructions
Typecheck and unit tests.