tsgo: Fix js-packages/shared-extension-utils type errors#47382
tsgo: Fix js-packages/shared-extension-utils type errors#47382manzoorwanijk wants to merge 2 commits intotrunkfrom
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
There was a problem hiding this comment.
Pull request overview
This PR converts two JavaScript utility functions to TypeScript to fix type errors in downstream packages. The conversion adds proper type definitions and replaces string-based WordPress store references with typed store descriptors for improved type safety.
Changes:
- Converted
get-jetpack-extension-availability.jsto TypeScript with exportedExtensionAvailabilityinterface - Converted
use-autosave-and-redirect/index.jsto TypeScript with exportedUseAutosaveAndRedirectReturninterface and proper event typing - Replaced string-based store references with typed store descriptors from
@wordpress/editor,@wordpress/core-data, and@wordpress/edit-widgets
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| projects/js-packages/shared-extension-utils/src/get-jetpack-extension-availability.ts | New TypeScript version with typed interfaces for extension availability checking |
| projects/js-packages/shared-extension-utils/src/get-jetpack-extension-availability.js | Removed JavaScript version (converted to TypeScript) |
| projects/js-packages/shared-extension-utils/src/hooks/use-autosave-and-redirect/index.ts | Converted to TypeScript with typed parameters, return interface, and store descriptors |
| projects/js-packages/shared-extension-utils/package.json | Added three WordPress dependencies for typed store access |
| projects/js-packages/shared-extension-utils/changelog/update-tsgo-fix-shared-extension-utils-type-errors | Changelog entry documenting the TypeScript conversion |
| pnpm-lock.yaml | Updated lockfile with new WordPress package dependencies |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (5)
projects/js-packages/shared-extension-utils/src/hooks/use-autosave-and-redirect/index.ts:29
- The window.top property can be null in certain scenarios (e.g., in sandboxed iframes with specific permissions). The type assertion
window.top as Windowbypasses TypeScript's null checking and could lead to a runtime error if window.top is null. Consider adding a null check before accessing the location property, or use optional chaining:window.top?.location.href = urlwith appropriate fallback handling.
projects/js-packages/shared-extension-utils/src/hooks/use-autosave-and-redirect/index.ts:7 - The noop function is typed as
() => {}which returns undefined, but it's used as the default value for the onRedirect parameter which expects( url: string ) => void. While this works at runtime, it would be more type-safe to define noop with the correct signature:const noop = ( _url: string ) => {};to match the expected callback signature.
projects/js-packages/shared-extension-utils/src/hooks/use-autosave-and-redirect/index.ts:58 - The type assertion for window is overly permissive with
{ wp?: { customize?: unknown } }. Since the code only checks for the existence of wp.customize without calling it or accessing its properties, a more precise type would be{ wp?: { customize?: object } }or even better, define a proper interface for the WordPress customizer API if it's used elsewhere in the codebase.
projects/js-packages/shared-extension-utils/src/hooks/use-autosave-and-redirect/index.ts:11 - The Event type used for the event parameters is less specific than the actual events passed to these handlers. Based on usage throughout the codebase (e.g., projects/js-packages/ai-client/src/hooks/use-ai-checkout/index.ts expects MouseEvent< HTMLButtonElement >), these functions are always called from onClick handlers which pass React.MouseEvent. Consider using React.MouseEvent instead of Event for better type safety and consistency with the rest of the codebase.
projects/js-packages/shared-extension-utils/src/hooks/use-autosave-and-redirect/index.ts:22 - The return type annotation in the JSDoc comment states "Window | null", but the function doesn't always return these types. When shouldOpenNewWindow is false, it returns the result of assigning to window.top.location.href (which is a string assignment expression). The return type should more accurately be "Window | null | string" or the JSDoc should be updated to match the actual behavior. Alternatively, consider refactoring to make the return value more consistent.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Convert get-jetpack-extension-availability.js and use-autosave-and-redirect/index.js to TypeScript with proper return type interfaces. Replace string-based store references with store descriptors for type-safe access. Fixes TS2339 errors in downstream packages (ai-client, publicize, jetpack).
b436ee9 to
2ad795a
Compare
Code Coverage Summary2 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| details?: Array< unknown >; | ||
| } | ||
|
|
||
| interface JetpackData { | ||
| available_blocks?: Record< string, BlockAvailability >; | ||
| } | ||
|
|
||
| export interface ExtensionAvailability { | ||
| available: boolean; | ||
| details?: Array< unknown >; | ||
| unavailableReason?: string; |
There was a problem hiding this comment.
Both the BlockAvailability.details field (line 6) and the ExtensionAvailability.details field (line 15) are typed as Array<unknown>, but the runtime value is a JSON object (PHP associative array), not a JavaScript array. This is confirmed by consumer code such as details.required_plan in plan-utils.js (line 109) and fieldFileAvailability?.details?.required_plan in field-file/edit.js. The correct type should be Record<string, unknown> (or a more specific shape like { required_plan?: string }).
Fixes https://linear.app/a8c/issue/MONOREP-378
Proposed changes:
get-jetpack-extension-availability.jsto TypeScript with typed return (ExtensionAvailabilityinterface), fixing TS2339 errors in downstream packages (ai-client, jetpack AI assistant blocks).use-autosave-and-redirect/index.jsto TypeScript with typed return (UseAutosaveAndRedirectReturninterface), fixing TS2339 errors in downstream packages (ai-client, jetpack AI assistant hooks).Replace string-based store references (Reverted due to build failure'core/editor','core','core/edit-widgets') with proper store descriptors (editorStore,coreStore,editWidgetsStore) for type-safe store access.Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
cd projects/js-packages/shared-extension-utils && pnpm typecheckChangelog