Conversation
aduth
added
[Type] Automated Testing
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` |
Member
Author
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
Contributor
|
👋 @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. |
sirreal
force-pushed
the
update/keycodes-types
branch
from
3261380 to
d9b3cf3
Compare
Member
|
I've rebased and pushed some changes so this should be ready. I think we can move ahead with it. |
sirreal
removed
the
[Status] Blocked
packages/keycodes/src/index.js
Outdated
| /** | ||
| * @template T | ||
| * | ||
| * @typedef {(event:KeyboardEvent,character:string,isApple?:()=>boolean)=>T} WPEventKeyHandler |
Contributor
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
Contributor
sarayourfriend
left a comment
sarayourfriend
left a comment
There was a problem hiding this comment.
LGTM, just some suggestions to improve readability.
packages/keycodes/src/index.js
Outdated
| * @typedef {Record<WPKeycodeModifier, (key:string)=>any>} WPKeycodeHandlerByModifier | ||
| * @template T | ||
| * | ||
| * @typedef {Record<WPKeycodeModifier,T>} WPModifierHandler |
Contributor
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 |
Contributor
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 |
Contributor
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 |
Member
|
@saramarcondes, can you take over this PR and land it? As far as I know, @aduth might not have time to finish it 😃 |
Contributor
|
@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 🙂 |
Member
|
Sure thing, it's nice to have, nothing urgent, thank you 😃 By the way, enjoy your time off 🎉 |
sarayourfriend
force-pushed
the
update/keycodes-types
branch
from
d9b3cf3 to
780a730
Compare
Contributor
Member
[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 |
sirreal
force-pushed
the
update/keycodes-types
branch
from
0c5c8f1 to
1846ff1
Compare
sirreal
force-pushed
the
update/keycodes-types
branch
from
0c5c8f1 to
1846ff1
Compare
gziolo
approved these changes
Dec 28, 2020
Member
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
I left some minor comments to check but otherwise it's good to go 👍
| - `shiftAlt` | ||
| _Type_ | ||
|
|
||
| - (unknown type) |
Member
There was a problem hiding this comment.
It looks like the documentation tool can't understand TS type ... 🙁
sirreal
force-pushed
the
update/keycodes-types
branch
from
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 */ |
Member
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
Member
sirreal
left a comment
sirreal
left a comment
There was a problem hiding this comment.
I'd like for this to be landed, I think it's been on hold long enough 🙂
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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: