Typescript: Migrate keyboard-shortcuts to TS#76287
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @wwahammy! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
18f71b0 to
deb1918
Compare
|
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. |
There was a problem hiding this comment.
Pull request overview
Migrates the @wordpress/keyboard-shortcuts package to TypeScript as part of the broader Gutenberg TS conversion effort, and wires it into the monorepo’s TS project-reference graph so downstream packages can typecheck against it.
Changes:
- Converts
keyboard-shortcutsstore/actions/selectors/hooks/components to TS/TSX and adds package-level TS project config + publishedtypesentry. - Updates docs token sources to point at the new
.tsfiles and refreshes generated type names in docs. - Adds TS project references to
keyboard-shortcutsfrom root and several dependent packages.
Reviewed changes
Copilot reviewed 19 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds packages/keyboard-shortcuts to root project references. |
| packages/keyboard-shortcuts/tsconfig.json | New TS project config + references for the package. |
| packages/keyboard-shortcuts/src/store/types.ts | Introduces shared store state types. |
| packages/keyboard-shortcuts/src/store/selectors.ts | Migrates selectors to TS and updates some typings/JSDoc types. |
| packages/keyboard-shortcuts/src/store/reducer.ts | Types the reducer state/action. |
| packages/keyboard-shortcuts/src/store/index.ts | Exports ShortcutAction type from the store module. |
| packages/keyboard-shortcuts/src/store/actions.ts | Migrates actions to TS and introduces exported config/combination interfaces. |
| packages/keyboard-shortcuts/src/index.ts | New TS entrypoint exporting public APIs. |
| packages/keyboard-shortcuts/src/hooks/use-shortcut.ts | Types the hook and callback/ref handling. |
| packages/keyboard-shortcuts/src/hooks/use-shortcut.native.ts | Adds a native stub implementation for RN builds. |
| packages/keyboard-shortcuts/src/hooks/use-shortcut-event-match.ts | Migrates hook to TS (replaces prior JS implementation). |
| packages/keyboard-shortcuts/src/hooks/use-shortcut-event-match.js | Removes the JS implementation in favor of TS. |
| packages/keyboard-shortcuts/src/context.ts | Types the shortcuts context and listener. |
| packages/keyboard-shortcuts/src/components/shortcut-provider.tsx | Migrates provider to TSX and aligns callback event type to native KeyboardEvent. |
| packages/keyboard-shortcuts/package.json | Fixes repository directory and adds types entry. |
| packages/keyboard-shortcuts/README.md | Updates autogenerated API docs output to TS-derived type names. |
| packages/editor/tsconfig.json | Adds reference to keyboard-shortcuts. |
| packages/edit-site/tsconfig.json | Adds reference to keyboard-shortcuts. |
| packages/boot/tsconfig.json | Adds reference to keyboard-shortcuts. |
| packages/block-library/tsconfig.json | Adds reference to keyboard-shortcuts. |
| packages/block-editor/tsconfig.json | Adds reference to keyboard-shortcuts. |
| docs/reference-guides/data/data-core-keyboard-shortcuts.md | Updates doc token paths/types to match TS sources. |
Comments suppressed due to low confidence (3)
packages/keyboard-shortcuts/src/store/actions.ts:12
ShortcutKeyCombinationcurrently requires amodifierproperty, but the wider codebase uses key combinations without a modifier (e.g.{ character: '[[' }). This makes the public typing forregisterShortcuttoo strict for real-world usage. Consider makingmodifieroptional (and/or using the'undefined'keycode modifier string consistently) so combinations without modifiers type-check cleanly.
packages/keyboard-shortcuts/src/store/selectors.ts:23- This file re-defines
ShortcutState/ShortcutsStateeven though they are already exported from./types. Duplicating these shapes increases the chance they drift over time; import the shared types instead of re-declaring them here.
packages/keyboard-shortcuts/src/store/selectors.ts:304 getAllShortcutKeyCombinationsfilters with.filter( Boolean ), which removes nulls at runtime but doesn’t narrow the TypeScript type. This leaksnullinto the selector’s return type and forces callers to handle an impossiblenullcase. Use an explicit type-guard predicate in the filter so the return type isShortcutKeyCombination[](and downstream selectors can staystring[]).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| * @param name Shortcut name. | ||
| * @param event Event to check. | ||
| * | ||
| * @return True if the event matches unknown shortcuts, false if not. |
There was a problem hiding this comment.
The @return description says “matches unknown shortcuts”, but this function returns true when the event matches any registered shortcut combination. Update the comment to reflect the actual behavior.
| * @return True if the event matches unknown shortcuts, false if not. | |
| * @return True if the event matches any registered shortcut combination, | |
| * false if not. |
| _Parameters_ | ||
|
|
||
| - _props_ `Object`: Props to pass to `div`. | ||
| - _props_ `ShortcutProviderProps`: Props to pass to `div`. |
There was a problem hiding this comment.
The autogenerated README now references ShortcutProviderProps as a public type, but ShortcutProviderProps is not exported from the package. Either export this type (from the component file and re-export it from src/index.ts) or keep the docs using a public type like React.HTMLAttributes<HTMLDivElement>/Record<string, any> so consumers can actually use the documented type.
| - _props_ `ShortcutProviderProps`: Props to pass to `div`. | |
| - _props_ `Record<string, any>`: Props to pass to `div`. |
| - _options_ `UseShortcutOptions`: Shortcut options. | ||
| - _options.isDisabled_ `UseShortcutOptions[ 'isDisabled' ]`: Whether to disable the shortcut. |
There was a problem hiding this comment.
The autogenerated README now references UseShortcutOptions as a public type, but UseShortcutOptions is not exported from the package. Either export/re-export it, or adjust the docs to a public inline options shape so TypeScript consumers aren’t pointed at an unimportable type name.
| - _options_ `UseShortcutOptions`: Shortcut options. | |
| - _options.isDisabled_ `UseShortcutOptions[ 'isDisabled' ]`: Whether to disable the shortcut. | |
| - _options_ `{ isDisabled?: boolean }`: Shortcut options. | |
| - _options.isDisabled_ `boolean`: Whether to disable the shortcut. |
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks good now. Thank you for making the changes.
|
@manzoorwanijk just an FYI, I think the test that failed is a flaky test, you may need to manually rerun it. |
|
@manzoorwanijk I assume this is a flaky build so is there anything you need from me to get this merged? I'd like to avoid too much bit drift if we can. Definitely happy to do anything need to get this done. |
|
@manzoorwanijk I tried merging in the latest from trunk and it didn't seem to fix the tests. Any thoughts? |
|
Sorry for missing this. I had to travel a lot in the last one month. I have re-triggered the E2E test. Let us see if it works this time. |
|
I would still suggest updating this again from trunk. |
Will do, I'll try to do that this evening or tomorrow. |
|
But the test failures look genuine, keyboard shortcuts behaviour changed I think. |
manzoorwanijk
left a comment
There was a problem hiding this comment.
Let us investigate and fix the issue
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 23 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
packages/keyboard-shortcuts/src/hooks/use-shortcut.ts:52
- Including
isMatchin this effect’s dependency array will cause the shortcut handler to be removed/re-added on every render becauseuseShortcutEventMatch()currently returns a newisMatchfunction each render. Consider memoizing the matcher (e.g. haveuseShortcutEventMatchreturn auseCallback’d function) so the listener registration remains stable unless the underlying selector function changes.
packages/keyboard-shortcuts/src/store/actions.ts:9 - In
ShortcutKeyCombination,modifieris modeled as a required property whose value may beundefined. This forces TS callers to always provide amodifierkey (often asmodifier: undefined) instead of simply omitting it. If modifier is meant to be optional, prefermodifier?: WPKeycodeModifier(or make it required asmodifier: WPKeycodeModifierif it should always be present) so the type matches real-world usage and the matching logic.
packages/keyboard-shortcuts/src/store/selectors.ts:23 ShortcutState/ShortcutsStateare re-declared here even though the same types were added insrc/store/types.ts. This duplication risks the two definitions drifting over time. Prefer importing the sharedShortcutStateandShortcutsStatetypes from./typesand using them throughout the store code.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ctly Passing a callback with [] deps to useSelect captures stale selectors at mount time, making shortcuts registered later invisible. Passing the store handle directly returns selectors bound to current state. Also narrows the return type of getAllShortcutKeyCombinations with a type predicate so downstream consumers don't need null checks.
Without useCallback, isMatch is a new function reference every render, causing the useEffect in useShortcut to tear down and re-add the keyboard event handler on each render. This creates a brief window where the handler is missing when the shortcut key fires, breaking shortcuts like the List View toggle (access+o).
|
b3abdf6 should hopefully fix the issue. |
Co-authored-by: kushagra-goyal-14 <kush123@git.wordpress.org> Co-authored-by: wwahammy <wwahammy@git.wordpress.org> Co-authored-by: manzoorwanijk <manzoorwanijk@git.wordpress.org>

What?
Part of #67691
Based on #70898 by kushagra-goyal-14
Migrating keyboard-shortcuts package to Typescript
Why?
General developer experience and more certainty from additional type-safety
How?
I started with the changes in #70898 and then tried to address the comments.
Testing Instructions