Add script which checks published types for non-checked JS packages #49736
Conversation
noahtallen
requested review from
ellatrix,
fluiddot,
dcalhoun and
atimmer
as code owners
|
Edit: turns out rememo didn't get updated in the package-lock file for some reason |
|
Size Change: +259 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in f0b7625. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4683294126
|
| @@ -13,7 +13,7 @@ const packageNames = fs.readdirSync( PACKAGES_DIR ).filter( ( file ) => { | |||
| module.exports = { | |||
| watchFolders: [ path.resolve( __dirname, '../..' ) ], | |||
| resolver: { | |||
| sourceExts: [ 'js', 'json', 'scss', 'sass', 'ts', 'tsx' ], | |||
| sourceExts: [ 'js', 'cjs', 'json', 'scss', 'sass', 'ts', 'tsx' ], | |||
There was a problem hiding this comment.
Attempts to fix this error that happens after updating rememo:
Error: While trying to resolve module `rememo` from file `/Users/runner/work/gutenberg/gutenberg/packages/blocks/src/store/selectors.js`, the package `/Users/runner/work/gutenberg/gutenberg/node_modules/rememo/package.json` was successfully found. However, this package itself specifies a `main` module field that could not be resolved (`/Users/runner/work/gutenberg/gutenberg/node_modules/rememo/rememo.cjs`. Indeed, none of these files exist:
* /Users/runner/work/gutenberg/gutenberg/node_modules/rememo/rememo.cjs(.native|.ios.js|.native.js|.js|.ios.json|.native.json|.json|.ios.scss|.native.scss|.scss|.ios.sass|.native.sass|.sass|.ios.ts|.native.ts|.ts|.ios.tsx|.native.tsx|.tsx)There was a problem hiding this comment.
It seems to be causing an issue with the react native build, unfortunately. I'll have to see if we need to do anything else to fix it
noahtallen
force-pushed
the
add-published-types-checker
branch
from
0c916b9
to
d715b0f
Compare
There was a problem hiding this comment.
Pre-emptive approval pending your own testing. This looks like a useful tool.
Is there a way this could falsely reject valid PRs? If that's the case we might want to double-check before merging that it's not in the CI pipeline and prevents other work from coming in.
Nice work - I think this will be a valuable tool.
I think it would only reject invalid PRs, but it is in the CI pipeline. My thought is that typescript itself isn’t catching an issue that really should have been caught, so any issues it finds should be fixed before merging. And probably would be related to code you’re working on, because all existing issues have been fixed. (There was just one this PR fixes) Any other scenarios you think could be a problem? |
if this is the case. we just need to make sure this doesn't become someone else's problem when they want to contribute unrelated code and suddenly they can't merge because there was a preexisting type error they didn't introduce. |
|
Neat tool!
Wondering what motivated you to work on it, though? Did you see instances of TS declaration files that had errors in CI/locally for any specific packages in the monorepo? Was this reported somewhere before? |
It runs in CI, so all of the relevant packages (which I think is just one or two) are getting checked already in the PR. Earlier in the PR, I had to fix an existing issue which was caught in GitHub actions (by updating rememo). It doesn't narrow down to just staged or PR-related changes, so it should be catching everything!
The "nice" thing is that this is just checking that published types are correct -- e.g. will a consumer of the package get a type error just by including them. So there are a huge number of type errors that won't get caught because they are all internal and never make it into the published types. E.g. any type error inside a function definition never makes it to the published build-types. Just errors in, say, the function signature such as the one you noted here. (I did verify this tool would catch that specific issue.) In other words, for someone to get this error on a PR, they would have to make a new change to cause it, such as changing or writing a new function signature that includes a type error. But you wouldn't get this error when modifying existing type errors that don't get published to build/types. So the goal of the script is less about internal type checking (which we'd need
Yeah, @dmsnell noticed an issue here, where an existing Typescript error in a function signature was getting published to the compiled build types. As a result, when you consumed the package as a dependency in a different project, Typescript doesn't catch these issues when generating the definition files in the first place because we're turning type checking off for JS files. We have to turn off So ultimately, this script lets us safely turn off |
Thanks for double-checking this. I'm probably overreacting based on fears about the accuracy of the existing types and what happens when someone now starts mixing JSDoc types with the published types. 👍 |
|
Alright, I'll move forward with this, and will happily follow-up based on any more feedback we get! |
|
Thanks @noahtallen 🙌 |
|
I think there could be another scenario where this might be useful, when node ./bin/packages/check-build-type-declaration-files.js
Incorrect published types for /Users/noahallen/source/gutenberg/packages/components/build-types/index.d.ts:
node_modules/react-colorful/dist/components/HexColorInput.d.ts(1,8): error TS1259: Module '"/Users/noahallen/source/gutenberg/node_modules/@types/react/index"' can only be default-imported using the 'esModuleInterop' flag
node_modules/react-colorful/dist/types.d.ts(1,8): error TS1259: Module '"/Users/noahallen/source/gutenberg/node_modules/@types/react/index"' can only be default-imported using the 'esModuleInterop' flag
packages/components/build-types/box-control/types.d.ts(84,54): error TS2339: Property 'event' does not exist on type 'void'.
packages/components/build-types/box-control/types.d.ts(85,53): error TS2339: Property 'event' does not exist on type 'void'.
packages/components/build-types/custom-gradient-picker/types.d.ts(5,13): error TS1192: Module '"/Users/noahallen/source/gutenberg/node_modules/@types/gradient-parser/index"' has no default export.
packages/components/build-types/toolbar/toolbar-button/index.d.ts(2,13): error TS1259: Module '"/Users/noahallen/source/gutenberg/node_modules/@types/react/index"' can only be default-imported using the 'esModuleInterop' flag
packages/components/build-types/view/component.d.ts(24,28): error TS2314: Generic type 'WordPressComponent' requires 3 type argument(s).The problem is, we're using Another issue is that this error: packages/components/build-types/view/component.d.ts(24,28): error TS2314: Generic type 'WordPressComponent' requires 3 type argument(s).Is ignored in the source code with However... the But it seems that if a 3rd party consumes one of our packages which uses BTW, the script in this PR doesn't impact any of this, but maybe we should extend it to handle more of these issues? |
What?
Adds a script which checks the final published declaration files for certain JS packages (ones which have
checkJsdisabled)Why?
When
checkJsis disabled,tsccan output declaration files which contain errors. The result is that every consumer gets those errors. We don't want to publish these errors, and it's useful to disable JS type checking. (For example, it would be hard to fix every JS type issue in a large existing package, but we'd still like to publish its types for people to consume.)This situation happens because
checkJs: falsewill basically disable these errors during the normaltscbuild, but it'll happily copy the offending lines into the published declaration files.How?
Run a script which finds every relevant package (ones which include a tsconfig and which set
checkJsto false), and then runtsc --noEmiton the main published declaration file. This basically makes sure the public API for the package types are correct.One potential issue is that this probably won't type check any declaration file which isn't recursively referenced by index.d.ts. Not sure if this is a real problem, since that would indicate that the type in question is just for internal use 🤔
This script then runs after the main typescript build.
Testing Instructions
npm run build:package-types(the fact that there is no error is a indication the script works as well)packages/core-data/build-types/index.d.tsto introduce an error (such as adding this line:export function(x?: string, y: number): void;)node bin/packages/check-build-type-declaration-files.js