Repository navigation
Core Commands: Refactor to TypeScript - #84385
kushagra-goyal-14 wants to merge 5 commits into
Conversation
Add native declarations and project references while preserving runtime behavior and existing comments.
🤖 PR meta 🤖🎉 PropsIf 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. Updated as activity occurs, without notifying anyone named here. Add the |
|
Hi @manzoorwanijk let me know if you have some recommendations or have some thoughts regarding this. |
| /** | ||
| * The router private APIs consumed by the command palette. | ||
| */ | ||
| export interface RouterPrivateApis { |
There was a problem hiding this comment.
This hand-written copy already drifts from the router: RouterProvider requires routes and matchResolverArgs, which index.tsx does not pass, so the mismatch is hidden rather than typed. core-data has a precedent: export type CoreDataPrivateApis = typeof lockedApis. Exporting a RouterPrivateApis type the same way from @wordpress/router (and making routes/matchResolverArgs optional there, since both are already handled when absent) would let this file drop the copy.
| * The router private APIs consumed by the command palette. | ||
| */ | ||
| export interface RouterPrivateApis { | ||
| RouterProvider: React.ComponentType< { |
There was a problem hiding this comment.
React.ComponentType/React.ReactNode resolve through the UMD global with no import, so lint:published-deps cannot see the dependency that the new @types/react peer is for. Prefer import type { ComponentType, ReactNode } from 'react';, like @wordpress/commands. If the router type suggestion above lands, this file (and possibly the peer) goes away.
| const commands = useMemo< CommandConfig[] >( () => { | ||
| if ( | ||
| ! canCreateTemplate || | ||
| // @ts-expect-error Preserve the existing boolean/string comparison during migration. |
There was a problem hiding this comment.
! templateType === 'wp_template_part' is always false, so this classic-theme guard never applies. Keeping the behavior is right for this PR, but could you open a follow-up issue (likely intent: templateType !== 'wp_template_part') and reference it in this comment so the @ts-expect-error does not become permanent?
| import { useAdminNavigationCommands } from './admin-navigation-commands'; | ||
| import { useSiteEditorNavigationCommands } from './site-editor-navigation-commands'; | ||
| import { unlock } from './lock-unlock'; | ||
| import type { CommandPaletteSettings } from './types'; |
There was a problem hiding this comment.
CommandPaletteSettings is now the public parameter type of initializeCommandPalette, but consumers cannot import it. Consider export type { AdminMenuCommand, CommandPaletteSettings } from './types';.
| /** | ||
| * The title fields used to prioritize entity search results. | ||
| */ | ||
| export interface SearchableEntityRecord { |
There was a problem hiding this comment.
SearchableEntityRecord is only the internal constraint for orderEntityRecordsBySearch; it fits better in private-types.ts (or next to the util) than in the public types file.
| return ( records ?? [] ).map( ( record ) => { | ||
| const command = { | ||
| const commands = useMemo< CommandConfig[] >( () => { | ||
| return ( records ?? [] ).map< CommandConfig >( ( record ) => { |
There was a problem hiding this comment.
Nit: the template loader types its records with getEntityRecords< WpTemplate | WpTemplatePart >, but this loader's getEntityRecords call (line 125) is untyped. getEntityRecords< Post | Page > from @wordpress/core-data would keep them consistent.
| // @ts-expect-error JavaScript callers can pass values outside the typed contract. | ||
| expect( orderEntityRecordsBySearch( {} ) ).toEqual( [] ); |
There was a problem hiding this comment.
Let us narrow this suppression down to the point where it applies. See my blog post about it - https://manzoorwani.dev/youre-probably-using-ts-expect-error-wrong/
What?
See #67691
Converts
@wordpress/core-commandssource files and its search-ordering tests to TypeScript, and adds native type declarations.Why?
Provide typed command palette settings, navigation commands, and search results, with declarations generated from the package source.
How?
.ts/.tsx, preserving existing comments and traceable Git history.@wordpress/commands.@types/reactpeer.No runtime changes. Existing hooks and navigation behavior are preserved.
Testing Instructions
npm run typecheck.npm run test:unit -- packages/core-commands.npm run build.npm run check:type-declarations.npm run lint:published-deps.npm run wp-env -- start.Testing Instructions for Keyboard
Use of AI Tools
Codex assisted with the TypeScript migration, review feedback analysis, tests, and validation.