Block Editor: Add BlockContext component to type-checking #22353
Conversation
aduth
added
Good First Review
|
The build caught an error, which I wasn't seeing locally when running Eventually I could reproduce it by first forcibly removing existing It seems there may have been some caching that was preventing the error from being logged. It's clear enough what the solution is (including the dependencies in cc @sirreal |
|
Size Change: +10 B (0%) Total Size: 833 kB
ℹ️ View Unchanged
|
The error was that the reference omitted? This results in the missing declarations when building a dependent module?
Aside: This is effective. It should be roughly analogous to
The references are the only way This has popped up a few times. We could probably lint for this by verifying that |
| @@ -4,10 +4,14 @@ | |||
| "rootDir": "src", | |||
| "declarationDir": "build-types" | |||
| }, | |||
| "references": [ | |||
| { "path": "../element" } | |||
| ], | |||
There was a problem hiding this comment.
This actually depends on a bunch of things:
gutenberg/packages/block-editor/package.json
Lines 26 to 48 in e00a485
We should add the packages that are typed already at least.
There was a problem hiding this comment.
This actually depends on a bunch of things:
That package as a whole does, sure. Currently we only type-check two files, for which this references set is accurate. I guess it can save us some future work if we just go ahead and add all of the dependencies now?
There was a problem hiding this comment.
This is interesting to consider…
I've been thinking of the TypeScript project as corresponding to the JavaScript project, but in this case it really doesn't. It's a small subset of the block-editor package. As such, it has a subset of the dependencies.
I don't feel strongly about this one way or the other.
|
@sirreal There may be a misunderstanding. My concern isn't that there is an error. My concern was that there was no error for me locally, but then the error appeared in continuous integration. And only after the series of steps above was I finally able to reproduce the error locally. tl;dr: I got a false negative when testing locally. |
|
I don't think there's a misunderstanding, but we may have different perspectives and not share the same concerns 🙂 Here's how I'm looking at this:
It seems that this all stems from a misconfiguration that we can attempt to prevent in the future or to accept as a risk not worth mitigating because the difficulty or maintenance of the mitigation outweighs the severity. Even on CI, it's possible that through other dependencies or simple race conditions, the dependencies would happen to be built by the time the declarations were required. Note that there's a tradeoff here. We could clean and rebuild types every run, but I've anecdotally observed that rebuilding from scratch can take upwards of forty seconds whereas reusing valid incremental builds is virtually instantaneous. If we decide to mitigate, how could we prevent issues like this?
I don't foresee this type of problem being very severe, but I may be wrong. This is mostly intuition. I'd expect problems of this sort either surface rather quickly or be harmless for the most part. |
|
🤷 I just want a type-checking command that I can trust. |
Closes #21666
This pull request seeks to opt in the BlockContext component (introduced in #21467) to be type-checked. It was previously planned for #21467 but removed due to blocking incompatibilities of #21767. These incompatibilities have since been resolved in #21781, so the type-checking can once again be enabled.
Testing Instructions:
Ensure type checking passes: