TypeScript: migrate annotations package to TS#70602
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. |
| const addAnnotationClassName = ( OriginalComponent: any ) => { | ||
| return ( withSelect as any )( |
There was a problem hiding this comment.
I would prefer to avoid these two any here, which means, we probably should first convert withSelect to typescript.
|
What is the status here? |
… function as withSelect is now migrated.
|
Hi @manzoorwanijk, I made some changes since it seems |
There was a problem hiding this comment.
Pull request overview
Migrates the @wordpress/annotations package implementation and tests to TypeScript as part of the broader “convert packages to TS” effort, adding explicit types for annotations, store actions/state, and format registration.
Changes:
- Adds a new TS project config for
packages/annotationsand wires it into the root TS project references. - Ports annotations store (actions/reducer/selectors) and related runtime modules (format + block filter) to TypeScript and introduces shared
types.ts. - Updates docs autogen tokens to point at the new
.tssources.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds packages/annotations as a TS project reference. |
| packages/annotations/tsconfig.json | New package-level TS config (rootDir, declarationDir, references). |
| packages/annotations/src/types.ts | Introduces core annotation/store/format TypeScript types. |
| packages/annotations/src/store/actions.ts | Ports store actions to TS, adds typed action union usage. |
| packages/annotations/src/store/reducer.ts | Ports reducer to TS and adds typed helpers/validation. |
| packages/annotations/src/store/selectors.ts | Adds TS types for selector inputs/outputs and tightens JSDoc. |
| packages/annotations/src/store/index.ts | Keeps store registration; minor JSDoc cleanup. |
| packages/annotations/src/store/constants.ts | Minor JSDoc cleanup. |
| packages/annotations/src/store/test/reducer.ts | Updates tests for TS types and stricter action shapes. |
| packages/annotations/src/index.ts | New TS entrypoint importing format/block side effects and exporting store. |
| packages/annotations/src/format/index.ts | TS adjustment for registering the annotation rich-text format. |
| packages/annotations/src/format/annotation.ts | Adds RichTextValue typing + TS-safe handling in format utilities. |
| packages/annotations/src/block/index.ts | Ports block filter to TS and uses the exported store in select. |
| docs/reference-guides/data/data-core-annotations.md | Updates autogen tokens to reference .ts sources. |
Comments suppressed due to low confidence (1)
packages/annotations/src/format/index.ts:13
registerFormatTypeis being called withsettings as any, which defeats the goal of the TS migration. It looks like theannotationformat object is missing required properties from theWPFormatshape (notablyinteractiveandobject); adding them to the format definition (and to theAnnotationFormattype) should allow removing this cast.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,20 @@ | |||
| { | |||
There was a problem hiding this comment.
The tsconfig files in this repo typically include the $schema field for editor/validation support. Consider adding it here for consistency with other package tsconfigs (e.g. packages/keycodes/tsconfig.json).
| { | |
| { | |
| "$schema": "https://json.schemastore.org/tsconfig", |
| * The `block` attribute refers to a block ID that needs to be annotated. | ||
| * `isBlockAnnotation` controls whether or not the annotation is a block | ||
| * annotation. The `source` is the source of the annotation, this will be used | ||
| * to identity groups of annotations. |
There was a problem hiding this comment.
The JSDoc says "to identity groups of annotations" which reads like a typo/grammar issue. This should likely be "to identify groups of annotations".
| * to identity groups of annotations. | |
| * to identify groups of annotations. |
| if ( selector === 'range' ) { | ||
| ( | ||
| action as Extract< AnnotationAction, { type: 'ANNOTATION_ADD' } > | ||
| ).range = range as WPAnnotationRange; |
There was a problem hiding this comment.
range is declared as WPAnnotationRange | null in AddAnnotationParameters, but this code force-casts it to WPAnnotationRange and assigns it even when it may be null. This weakens type safety and can hide invalid dispatches; consider only setting action.range when range is non-null (or update the AnnotationAction type to reflect null if it's intentionally allowed).
| if ( selector === 'range' ) { | |
| ( | |
| action as Extract< AnnotationAction, { type: 'ANNOTATION_ADD' } > | |
| ).range = range as WPAnnotationRange; | |
| if ( selector === 'range' && range !== null ) { | |
| ( | |
| action as Extract< AnnotationAction, { type: 'ANNOTATION_ADD' } > | |
| ).range = range; |
| attributes: { | ||
| className: string; | ||
| id: string; | ||
| }; |
There was a problem hiding this comment.
AnnotationFormat doesn't include the interactive and object properties that @wordpress/rich-text's registerFormatType expects for a WPFormat, which is likely why settings as any is needed in format/index.ts. Consider adding these fields (e.g. as booleans) so the format object can be passed to registerFormatType without an unsafe cast.
| }; | |
| }; | |
| interactive: boolean; | |
| object: boolean; |
| * @property {number} start The offset where the annotation should start. | ||
| * @property {number} end The offset where the annotation should end. | ||
| */ | ||
|
|
||
| /** |
There was a problem hiding this comment.
This standalone JSDoc block with only @property tags looks like a leftover from the old @typedef WPAnnotationRange and is no longer attached to any symbol (since WPAnnotationRange is now a TS interface). Consider removing it to avoid confusing doc generation / JSDoc linting.
| * @property {number} start The offset where the annotation should start. | |
| * @property {number} end The offset where the annotation should end. | |
| */ | |
| /** |
|
@manzoorwanijk I have made the changes as per the Copilot review. Can you please re-check? Thanks |
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks good. I have some suggestions for further improvements.
| "compilerOptions": { | ||
| "rootDir": "src", | ||
| "declarationDir": "build-types" | ||
| }, |
There was a problem hiding this comment.
We don't need these overrides.
| "compilerOptions": { | |
| "rootDir": "src", | |
| "declarationDir": "build-types" | |
| }, |
| ], | ||
| "include": [ "src/**/*" ], | ||
| "exclude": [ | ||
| "src/**/*.test.ts", | ||
| "src/**/*.test.tsx", | ||
| "src/**/test/**/*.ts", | ||
| "src/**/test/**/*.tsx" | ||
| ] |
There was a problem hiding this comment.
We don't need these overrides either
| ], | |
| "include": [ "src/**/*" ], | |
| "exclude": [ | |
| "src/**/*.test.ts", | |
| "src/**/*.test.tsx", | |
| "src/**/test/**/*.ts", | |
| "src/**/test/**/*.tsx" | |
| ] | |
| ] |
| selector: 'range', | ||
| range: { | ||
| start: 'not a number', | ||
| start: 'not a number' as any, |
There was a problem hiding this comment.
It's better to use // @ts-expect-error in such cases with a comment explaining why.
| range: { | ||
| start: 100, | ||
| end: 'not a number', | ||
| end: 'not a number' as any, |
There was a problem hiding this comment.
Same here about using // @ts-expect-error.
| /** | ||
| * Represents a range in text content. | ||
| */ | ||
| export interface WPAnnotationRange { |
There was a problem hiding this comment.
Let us remove the WP prefix here.
| /** | ||
| * Base annotation interface. | ||
| */ | ||
| export interface WPAnnotation { |
| "references": [ | ||
| { "path": "bin" }, | ||
| { "path": "packages/a11y" }, | ||
| { "path": "packages/annotations" }, |
There was a problem hiding this comment.
Let us place it in the correct place in alphabetical order.
manzoorwanijk
left a comment
There was a problem hiding this comment.
We haven't added the types field to package.json here. We also need to add it to the exports['.'] field there.
|
@claude Review this |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
packages/annotations/src/store/selectors.ts:76
- In
__experimentalGetAnnotationsForRichText,rangeis typed asAnnotationRange | null | undefined(sinceAnnotation.rangeis optional/nullable), but the code spreads it ({ ...range, ...other }). WithstrictTS this will error because object spread requires a non-null object type. Consider narrowing in the filter (e.g., ensureannotation.rangeis present) or otherwise makingrangenon-null before spreading so this selector typechecks and matches the reducer invariant (range annotations always have a range).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export interface AnnotationsState { | ||
| [ blockClientId: string ]: Annotation[]; | ||
| } |
There was a problem hiding this comment.
AnnotationsState is currently defined with a string index signature returning Annotation[], which implies every key lookup returns an array even when the blockClientId key is missing. The rest of the code already treats missing keys as undefined (e.g. state?.[ blockClientId ] ?? []), so the state type is unsound. Consider changing it to something like Record<string, Annotation[] | undefined> (or Partial<Record<string, Annotation[]>>) to reflect runtime behavior and avoid incorrect assumptions in consumers.
| export interface AnnotationsState { | |
| [ blockClientId: string ]: Annotation[]; | |
| } | |
| export type AnnotationsState = Partial< Record< string, Annotation[] > >; |
…notationsState interface
|
Let us make CI happy and please mention me when it's ready again. Thank you for your contributions. |
…xperimentalGetAnnotations to filter out undefined values in selectors.ts
|
Hi @manzoorwanijk, I have made the changes, just waiting for the CI. |
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks good now. Thank you.

What?
Part of #67691
Migrating the annotations package to TypeScript.
Why?
To enhance DX and add type safety.
How?
By porting the code and tests to TypeScript.
Testing Instructions
Typecheck and unit tests.