Keycodes: Add, enhance types #19520
Merged
Keycodes: Add, enhance types #19520
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aduth
commented
Jan 9, 2020
packages/keycodes/README.md
Outdated
Comment on lines
115
to
143
| - `primary`: takes a isApple function as a parameter. | ||
| - `primaryShift`: takes a isApple function as a parameter. | ||
| - `primaryAlt`: takes a isApple function as a parameter. | ||
| - `secondary`: takes a isApple function as a parameter. | ||
| - `access`: takes a isApple function as a parameter. | ||
| - `ctrl` | ||
| - `alt` | ||
| - `ctrlShift` | ||
| - `shift` | ||
| - `shiftAlt` | ||
| _Type_ | ||
|
|
||
| - `null` |
There was a problem hiding this comment.
This is what I am referring to with the remarks in "Current Status" of the original comment.
This was referenced Jan 9, 2020
|
👋 @aduth ! Just wanted to check and see if there was anything blocking this work or if it just needs to get brought up to date. I'm investigating adding types to a mobile package that depends on the keycodes package, so I'd like to help get this merged if possible. I'd be happy to bring this branch up to date (either by committing directly or PR'ing into this branch) if you'd like. |
3261380 to
d9b3cf3
Compare
|
I've rebased and pushed some changes so this should be ready. I think we can move ahead with it. |
packages/keycodes/src/index.js
Outdated
| /** | ||
| * @template T | ||
| * | ||
| * @typedef {(event:KeyboardEvent,character:string,isApple?:()=>boolean)=>T} WPEventKeyHandler |
There was a problem hiding this comment.
Is this any old KeyboardEvent or is it going to be React's?
|
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
sarayourfriend
approved these changes
Nov 27, 2020
packages/keycodes/src/index.js
Outdated
| * @typedef {Record<WPKeycodeModifier, (key:string)=>any>} WPKeycodeHandlerByModifier | ||
| * @template T | ||
| * | ||
| * @typedef {Record<WPKeycodeModifier,T>} WPModifierHandler |
There was a problem hiding this comment.
Suggested change
| * @typedef {Record<WPKeycodeModifier,T>} WPModifierHandler | |
| * @typedef {Record<WPKeycodeModifier, T>} WPModifierHandler |
packages/keycodes/src/index.js
Outdated
| /** | ||
| * @template T | ||
| * | ||
| * @typedef {(character:string,isApple?:()=>boolean)=>T} WPKeyHandler |
There was a problem hiding this comment.
Suggested change
| * @typedef {(character:string,isApple?:()=>boolean)=>T} WPKeyHandler | |
| * @typedef {(character: string, isApple?: () => boolean) => T} WPKeyHandler |
packages/keycodes/src/index.js
Outdated
| /** | ||
| * @template T | ||
| * | ||
| * @typedef {(event:KeyboardEvent,character:string,isApple?:()=>boolean)=>T} WPEventKeyHandler |
There was a problem hiding this comment.
Suggested change
| * @typedef {(event:KeyboardEvent,character:string,isApple?:()=>boolean)=>T} WPEventKeyHandler | |
| * @typedef {(event: KeyboardEvent, character: string, isApple?: () => boolean) => T} WPEventKeyHandler |
|
@saramarcondes, can you take over this PR and land it? As far as I know, @aduth might not have time to finish it 😃 |
|
@gziolo I'm happy to do that, however I'm going to be mostly away from my computer for the next week and a half. I should have some time here or there to take care of this, just won't happen right away if that's okay 🙂 |
|
Sure thing, it's nice to have, nothing urgent, thank you 😃 By the way, enjoy your time off 🎉 |
d9b3cf3 to
780a730
Compare
[2] ✖ prefer-property-order - node: - Your package.json properties are not in the desired order. Please move "types" after "react-native".
29
[2] 1 error
30
[2] 0 warningsIt's difficult to process this output from CI, but it's a simple change in the |
0c5c8f1 to
1846ff1
Compare
gziolo
approved these changes
Dec 28, 2020
| - `shiftAlt` | ||
| _Type_ | ||
|
|
||
| - (unknown type) |
There was a problem hiding this comment.
It looks like the documentation tool can't understand TS type ... 🙁
1846ff1 to
198bb1a
Compare
sirreal
reviewed
Dec 29, 2020
| /** | ||
| * @typedef {'primary'|'primaryShift'|'primaryAlt'|'secondary'|'access'|'ctrl'|'alt'|'ctrlShift'|'shift'|'shiftAlt'} WPKeycodeModifier | ||
| */ | ||
| /** @typedef {typeof ALT | CTRL | COMMAND | SHIFT } WPModifierPart */ |
There was a problem hiding this comment.
Another disadvantage to JSDoc is that these types will be public. They cannot be declared as typedefs here without being exported.
sirreal
approved these changes
Dec 29, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of: #18838
This pull request seeks to enable type checking for the
@wordpress/keycodespackage.Current Status:
As we start to use more TypeScript-specific types, the ability to document these sufficiently is beginning to suffer, due to the fact that the current Doctrine parser cannot make sense of these types. The issue at #18045 tracks the ongoing effort to replace the (defunct) Doctrine project with an alternative implementation (which I've proposed to be the TypeScript parser). In the meantime, however, the generated documentation suffers. Worse than if it simply did not document the types at all, it documents them as the
nulltype, which is flatly misleading. Therefore, I'd want to try to come to a solution to this problem before pushing forward with much more use of the custom TypeScript types.Testing Instructions:
Verify no regressions in modified behavior:
Verify types checking passes: