Priority Queue: Enable types with enhanced JSDoc type details #18997
Conversation
aduth added [Type] Code Quality 
| @@ -79,6 +79,7 @@ | |||
| "@storybook/addon-storysource": "5.2.4", | |||
| "@storybook/addon-viewport": "5.2.4", | |||
| "@storybook/react": "5.2.4", | |||
| "@types/requestidlecallback": "0.3.1", | |||
There was a problem hiding this comment.
Is this a prod or dev dependency?
There was a problem hiding this comment.
Is this a prod or dev dependency?
Should be development-only, I think.
| @@ -40,7 +40,7 @@ queue.add( ctx2, () => console.log( 'This will be printed second' ) ); | |||
| _Returns_ | |||
| - `Object`: Queue object with `add` and `flush` methods. | |||
| - `WPPriorityQueue`: Queue object with `add` and `flush` methods. | |||
There was a problem hiding this comment.
Aside: Enhancements resulting from #15186 could be an important selling-point in convincing developers why documenting custom types could be beneficial for improved, automated documentation.
Aside: I think some of the difficulty is self-imposed in trying to retroactively apply types details to existing code. In this case, there's some additional complications around how we overload use of The optimist in me would like to think that if we start to approach new code with types in mind, this process could coerce us into making better choices about writing simpler code, by virtue of the "pain" associated with defining types for complex implementations. |
Part of: #18838
This pull request seeks to enable type checking for the
@wordpress/priority-queuepackage.Implementation Notes:
A few details here worth noting, as it can impact future types efforts:
requestIdleCallback) or otherwise-unsupportedwindowglobals. In this case, it's fortunate there's a DefinitelyTyped package for@types/requestidlecallback. In other circumstances, we may need to consider how this be handled (e.g. allowable.d.tsfiles for augmenting the global scope, which might be the only option).waitingList.shift(). This is apparently a common problem, where TypeScript-proper affords some of its own options (the non-null assertion operator). These inline type-castings may be a suitable alternative, but they're reasonably clumsy to work with.valid-jsdoc. See also related issue at Typescript types should parse correctly jsdoctypeparser/jsdoctypeparser#50 . To me, it seems like something where a future version ofjsdoctypeparser(eslint-plugin-jsdoc) will have better support for these syntaxes, but we may need to be willing to allow for the warnings in the interim. It could be an open question as to whether we want to includeeslint-disableinline configurations to avoid the warnings.@typetag when documenting the type of an inline variable (e.g./** @type {WPPriorityQueueContext[]} */)? I worry that if we force these JSDoc to occupy at least three lines each, there might be some reluctance/resistance due to its verbosity.WPPriorityQueueAdd, etc. I think this is something where we'll want to strike a balance of pragmatism on what's reasonable to document. An exhaustive set of descriptions here might be difficult to achieve, and provide minimal value vs. an adequate description on the parameters where the types are referenced. To me, it can also be one of those things where we approach it from a mindset of iterative improvement over time, and avoid added barriers to introducing types.Testing Instructions:
Verify types checking passes:
There are no unit tests for
@wordpress/priority-queue, so it might be worth doing some brief user-testing of the affected usage (useSelectuses the priority queue).