Size Change: 0 B

Total Size: 1.64 MB

_{compressed-size-action}

Have you tried updating the existing ESLint config to check missing type imports?

https://github.com/WordPress/gutenberg/blob/1dd0c9d13e16428ee27ba71a04d7ec331566ad01/packages/eslint-plugin/configs/recommended-with-formatting.js#L26L31

According to https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-extraneous-dependencies.md#options:

Type imports can be verified by specifying includeTypes.

Have you tried updating the existing ESLint config to check missing type imports?

Not yet, but this is a great suggestion!

I did a typecheck on the components build-types, and got these results:

 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

I don't understand why the normal tsc pass doesn't catch these issues. In components, checkJs is true, and we also get these issues when we disable skipLibCheck.

There are at least two issues in our code that tsc probably should complain about:

• @types/gradient-parser has no default export: We are importing type gradientParser directly from the package, but it has no default export, which obviously doesn't work. Nothing detects this issue -- you might be able to use import type * as ..., or just import the named exports, but nothing tells you to do that.
• Property 'event' does not exist on type 'void'.: We're trying to use the return type of a function. We get a property off of the return type and use that -- but the return type can sometimes be void, which means sometimes, that property doesn't exist. Not sure why tsc doesn't catch this earlier.

But with the various react import issues, 3rd party transitive dependencies are importing the default export react directly as a type, which seemingly doesn't work with our module resolution settings. Not complained about in our code, but could be an issue for consumers.

Flaky tests detected in a52f974.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6410764586
📝 Reported issues:

• [Flaky Test] should partially select with shift + click (@webkit) #53524 in /test/e2e/specs/editor/various/multi-block-selection.spec.js

By looking at the code changes, this should have been solved, right?

Right, I added the fix for this.

so probably the most robust solution is to somehow improve the module resolution at the project level.

I'm not sure how to accomplish this. Taking a guess, React is a peer dependency of components. So when I'm typechecking build-types, perhaps tsc doesn't understand it's operating in a monorepo and that React is provided at the top level. So maybe this doesn't cause issues for 3rd parties. I think either way, we don't have to address it in this PR

Sounds good!

From what I understand, there may still be a few issues errors:

1. The "Property 'event' does not exist on type 'void'." error, likely caused by these types
2. ToolbarButton importing React directly — we should be able to delete the line without consequences and get rid of the error
3. the react-colorful error about importing @types/react, which from your previous message, we don't have a solution for yet

What are the next steps? Should be at least fix errors from point 1 and 2 before merging?

Also curious to hear @tyxla and @mirka 's thoughts as they initially followed this tasks more closely than I did.

What @ciampo suggested sounds like a good plan to me 👍 We can follow up with fixing the third one if it's more time-taking.

I noticed this on my backlog, and I think it's pretty much ready to go. The main thing we had to add in wp-calypso was the extra @types dependencies. I think many of the other errors might not happen in 3rd party repos (or at least don't happen to us). It's hard to say. So I think the current PR would be an improvement.

• The "Property 'event' does not exist on type 'void'." error -- may not impact 3rd party repos.
• ToolbarButton importing React directly — no longer an issue
• about importing @types/react -- may not impact 3rd party repos.
• Certain @types packages not bundled in npm -- fixed

The reason I say they may not impact 3rd party repos is because we haven't seen those errors in our installation of @wordpress/components. I think the @types packages is the only workaround we have to be using current versions of wp components.