Publish build types for notices #49650
Merged
Conversation
This file contains 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
noahtallen
commented
Apr 7, 2023
| @@ -14,8 +14,6 @@ import * as selectors from './selectors'; | |||
| * Store definition for the notices namespace. | |||
| * | |||
| * @see https://github.com/WordPress/gutenberg/blob/HEAD/packages/data/README.md#createReduxStore | |||
| * | |||
| * @type {Object} | |||
There was a problem hiding this comment.
This isn't really useful by itself, and when we remove it, typescript will automatically infer the entire type of the store, including actions/selectors/etc
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 6ed3253. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4640689821
|
tyxla
reviewed
Apr 7, 2023
dmsnell
approved these changes
Apr 10, 2023
There was a problem hiding this comment.
The generated types are interesting, and I don't think I explored this exact approach with checkJs: false
Please check something out though before merging if you will: we carried over what appears to be a type error in createNotice, where the first parameter status is optional but then is followed by a non-optional content parameter.
Let's make sure that code importing this package won't have to face that type error and be falsely rejected.
Thanks @noahtallen - if this model works well I think it could be a good path forward to eliminating the Definitely Typed types altogether.
Good point, will try to check this. |
The odd things about this is that gutenberg/packages/notices/src/store/actions.js Lines 19 to 70 in af7da80 So it seems typescript could have done a better job converting it |
|
It didn't work: node_modules/@wordpress/notices/build-types/store/actions.d.ts(52,59): error TS1016: A required parameter cannot follow an optional parameter. |
|
Similar issue in the Typescript repo: microsoft/TypeScript#47025 (comment) |
|
Summarizing:
While for notices, it might be easy to fix those 11 errors (assuming we can do it in jsdoc without much hassle), that could be much harder for the larger packages. I wonder if we could somehow check these declaration files after the fact to see if they contain errors? Then we can easily fix anything that would ultimately get published. |
|
I tried this command, and it does correctly identify the issue in the published type file: npx tsc --noEmit ./packages/notices/build-types/index.d.ts
packages/notices/build-types/store/actions.d.ts:52:59 - error TS1016: A required parameter cannot follow an optional parameter.
52 export function createNotice(status?: string | undefined, content: string, options?: {
~~~~~~~
Found 1 error in packages/notices/build-types/store/actions.d.ts:52Soooo I wonder if we could add this to our toolchain. Where we basically run |
|
I'd like to see how far we can get with that, and use it to fix obvious errors. This is the whole conundrum with the divide. Types in Core are often wrong. Types in Definitely Typed are often of little value, but rarely do they product false negatives. IMO it's impractical to fix all type errors in Core before publishing types, but if we're able to lie enough that we don't trigger false positives I think that would be fine and pave a long-term path for improving the quality of the types. |
|
This should do it: #49736 |
noahtallen
added
[Type] Build Tooling
noahtallen
force-pushed
the
publish-notices-build-types
branch
from
6ed3253 to
b1bf4dc
Compare
|
After rebasing with the new script, we can see it catching this issue in the published types (https://github.com/WordPress/gutenberg/actions/runs/4739254805/jobs/8413911839?pr=49650#step:6:10) |
noahtallen
requested review from
ryanwelcher,
juanmaguitar and
fabiankaegy
as code owners
noahtallen
commented
Apr 19, 2023
| @@ -19,7 +19,7 @@ let uniqueId = 0; | |||
| /** | |||
| * Returns an action object used in signalling that a notice is to be created. | |||
| * | |||
| * @param {string} [status='info'] Notice status. | |||
| * @param {string|undefined} status Notice status ("info" if undefined is passed). | |||
There was a problem hiding this comment.
The status='info' syntax makes "status" an optional argument in JSDoc. This actually isn't true -- status is still a required parameter. It's just set to "info" if undefined is passed as the first argument.
tyxla
approved these changes
Apr 20, 2023
noahtallen
force-pushed
the
publish-notices-build-types
branch
from
c2fff09 to
8d474bc
Compare
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.
What?
Publishes build types for the notices store
Why?
See #49647
How?
Adding a tsconfig and fixing anything that comes up! In this case, not much was needed.
Testing Instructions
CI, check build-types directory