Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Priority Queue: Enable types with enhanced JSDoc type details #18997

Merged
merged 4 commits into from Dec 11, 2019

Conversation

@aduth
Copy link
Member

aduth commented Dec 7, 2019

Part of: #18838

This pull request seeks to enable type checking for the @wordpress/priority-queue package.

Implementation Notes:

A few details here worth noting, as it can impact future types efforts:

  • Dealing with experimental DOM features (requestIdleCallback) or otherwise-unsupported window globals. 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.ts files for augmenting the global scope, which might be the only option).
  • Dealing with known non-nullables. See the required revisions to typecast the result of 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.
  • Type intersections cause an ESLint warning for valid-jsdoc. See also related issue at jsdoctypeparser/jsdoctypeparser#50 . To me, it seems like something where a future version of jsdoctypeparser (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 include eslint-disable inline configurations to avoid the warnings.
  • A few potential convention discussions emerge here:
    • Can we be okay for a condensed single-line @type tag 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.
    • Can we be okay to omit descriptions for some custom types? See affected 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:

npm run lint:types

There are no unit tests for @wordpress/priority-queue, so it might be worth doing some brief user-testing of the affected usage (useSelect uses the priority queue).

@@ -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",

This comment has been minimized.

Copy link
@youknowriad

youknowriad Dec 9, 2019

Contributor

Is this a prod or dev dependency?

This comment has been minimized.

Copy link
@aduth

aduth Dec 9, 2019

Author Member

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.

This comment has been minimized.

Copy link
@aduth

aduth Dec 9, 2019

Author Member

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.

Copy link
Contributor

youknowriad left a comment

LGTM 👍 I admit my brain is still not capable yet of scanning these types easily, so this is a very light review.

@aduth

This comment has been minimized.

Copy link
Member Author

aduth commented Dec 9, 2019

LGTM 👍 I admit my brain is still not capable yet of scanning these types easily, so this is a very light review.

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 requestAnimationFrame and requestIdleCallback. There was also a bit of ambiguity around what exactly we were expecting a context object to be (technically it could be an array... or an object... with or without any properties). A nice specific observation here is there's a bit more clarity around what the deadline argument is; previously, we did some simple property checks, and it wasn't entirely clear that the reason we were doing this is because it could either be the number parameter of a requestAnimationFrame, or the deadline parameter of a requestIdleCallback.

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.

@aduth aduth merged commit f31ae1c into master Dec 11, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@aduth aduth deleted the add/priority-queue-types branch Dec 11, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.