Publish types for plugins package #49649
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 6, 2023
noahtallen
commented
Apr 6, 2023
noahtallen
commented
Apr 6, 2023
|
Size Change: +2 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in d78218c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4640724747
|
tyxla
reviewed
Apr 7, 2023
There was a problem hiding this comment.
Great work @noahtallen 🙌
I we could aim to get this closer to the existing DT types, at least when it comes to precision. I've left some suggestions, let me know what you think.
Also, let's not forget to add some CHANGELOG entries for a breaking change where applicable (in the plugins package most certainly).
|
Another fantastic example of a rabbit hole: In order to fix all the type errors and start checking JS, we need to convert the I did a fair amount of digging yesterday, and couldn't figure out why this one thing causes an issue in this file, but not others. The solution is to keep the types in the docblock, instead of removing them. Definitely not ideal :/ Beyond that, the |
|
I also don't think the components change justifies a changelog entry here :p |
|
I wasn't sure if those errors would just occur locally, but it does look like they spill over here. Would appreciate help figuring them out since they seem completely unrelated :/ |
dmsnell
reviewed
Apr 10, 2023
I haven't had a chance to take a look at these errors myself, but this might indicate that there are existing TS errors in the components package that for some reason are manifesting only when we start explicitly exporting types. cc @ciampo @mirka and @chad1008 in case they have any idea what might be going on. |
mirka
reviewed
Apr 11, 2023
There was a problem hiding this comment.
Beyond that, the
npm run build:package-typescommand will continue outputting errors in unrelated packages like components, and even dependencies like reakit and popper. Who knows why, but when disabling checkJS, it goes away :/
I looked into this, and it's due to an upstream issue in Reakit. It's already been fixed in reakit@1.3.9, so I'll see if I can update our dependency version asap (currently 1.3.8). The quick fix is to add "skipLibCheck": true in the plugins/tsconfig.json.
noahtallen
force-pushed
the
publish-plugin-types
branch
from
fb2bf89 to
d891337
Compare
|
Alright, updated the PR:
|
adamziel
reviewed
Apr 13, 2023
dmsnell
reviewed
Apr 13, 2023
dmsnell
reviewed
Apr 13, 2023
tyxla
reviewed
Apr 13, 2023
noahtallen
force-pushed
the
publish-plugin-types
branch
from
d891337 to
6ea0e84
Compare
This was referenced Apr 13, 2023
noahtallen
force-pushed
the
publish-plugin-types
branch
2 times, most recently
from
07eed21 to
c507730
Compare
adamziel
reviewed
Apr 20, 2023
| * @param {string|undefined} props.scope | ||
| * @param {Function|undefined} props.onError | ||
| * @param props | ||
| * @param props.scope |
There was a problem hiding this comment.
Do you have to list all the properties now that they are statically typed? AFAIR other places just say @param props
There was a problem hiding this comment.
My IDE automatically inserts them on save, so I think yes?
adamziel
approved these changes
Apr 20, 2023
tyxla
approved these changes
Apr 20, 2023
Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
noahtallen
force-pushed
the
publish-plugin-types
branch
from
c507730 to
2b8575c
Compare
|
Do y'all think that this PR (and the related ones publishing types) deserve a dev note? |
ciampo
added
the
Needs Dev Note
I think that is a great idea. @noahtallen is this something you're planning to work on? I'm happy to collaborate there. |
24 tasks
6.3 Dev Note:Typescript developers appreciate having great in-editor type-hints and compiler warnings. We would also like to reduce our reliance on DefinitelyTyped, as it's much harder to maintain type information separately from the source code. As a result, we are publishing more types for various With some small tweaks to assist its inference capabilities, Typescript can generate extensive type definitions even for store objects without migrating much code to Typescript. So far, we have published types for the following packages using this process:
With work in progress on some larger packages such as |
noahtallen
added
has dev note
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?
I'm publishing types for more packages to avoid having to maintain stuff on DT.
Why?
See #49647
How?
Add the tsconfig and fix anything that comes up! Also check final build-types directory.
Testing Instructions
CI