Add some @types packages as proper dependencies#50231
Merged
noahtallen merged 3 commits intotrunkfrom Oct 4, 2023
Merged
Conversation
noahtallen
added
[Type] Build Tooling
|
Size Change: 0 B Total Size: 1.64 MB ℹ️ View Unchanged
|
tyxla
reviewed
May 2, 2023
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
The change makes sense to me 👍
I wonder why we need @types/gradient-parser in the first place.
Member
|
Have you tried updating the existing ESLint config to check missing type imports? According to https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/no-extraneous-dependencies.md#options:
|
Member
Author
Not yet, but this is a great suggestion! |
25 tasks
noahtallen
force-pushed
the
add-main-type-dependencies-for-components
branch
from
1f34bd4 to
09a6c1d
Compare
Member
Author
|
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 |
Member
Author
|
I don't understand why the normal There are at least two issues in our code that
But with the various react import issues, 3rd party transitive dependencies are importing the default export |
|
Flaky tests detected in a52f974. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6410764586
|
ciampo
reviewed
Jun 6, 2023
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Hey @noahtallen, I may have lost touch with this PR (and Lena is AFK until next week).
@types/gradient-parser has no default export
By looking at the code changes, this should have been solved, right?
Property 'event' does not exist on type 'void'.
Your explanation makes sense, it looks like it's indeed caused by these types. We will need to take a look an understand how to get around that.
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.
The error in ToolbarButton could be likely caused by this line — we could delete it and get rid of the error.
But we would still be left with the react-colorful error — so probably the most robust solution is to somehow improve the module resolution at the project level.
I don't understand why the normal
tscpass doesn't catch these issues. In components,checkJsis true, and we also get these issues when we disableskipLibCheck.
This is the most worrying aspect, IMO. We should definitely get tsc to link our files and our builds reliably.
| * Array of search words. String search terms are automatically cast to RegExps unless `highlightEscape` is true. | ||
| */ | ||
| highlightSanitize?: import('highlight-words-core').FindAllArgs[ 'sanitize' ]; | ||
| highlightSanitize?: FindAllArgs[ 'sanitize' ]; |
Contributor
There was a problem hiding this comment.
Not sure if it should be part of this PR or done as a follow-up, but just wanted to note that highlight-words-core is also used by packages/components/src/text/utils.js, which at this point could do with a TS refactor
ciampo
reviewed
Jun 6, 2023
Contributor
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Hey @noahtallen, I may have lost touch with this PR (and Lena is AFK until next week).
@types/gradient-parser has no default export
By looking at the code changes, this should have been solved, right?
Property 'event' does not exist on type 'void'.
Your explanation makes sense, it looks like it's indeed caused by these types. We will need to take a look an understand how to get around that.
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.
The error in ToolbarButton could be likely caused by this line — we could delete it and get rid of the error.
But we would still be left with the react-colorful error — so probably the most robust solution is to somehow improve the module resolution at the project level.
I don't understand why the normal
tscpass doesn't catch these issues. In components,checkJsis true, and we also get these issues when we disableskipLibCheck.
This is the most worrying aspect, IMO. We should definitely get tsc to link our files and our builds reliably.
Member
Author
Right, I added the fix for this.
I'm not sure how to accomplish this. Taking a guess, React is a peer dependency of components. So when I'm typechecking |
noahtallen
force-pushed
the
add-main-type-dependencies-for-components
branch
from
b50df4b to
ea95cb3
Compare
Contributor
|
Sounds good! From what I understand, there may still be a few issues errors:
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. |
Member
|
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. |
noahtallen
requested review from
ellatrix,
fabiankaegy,
juanmaguitar,
ndiego,
nerrad,
ntwb and
ryanwelcher
as code owners
noahtallen
force-pushed
the
add-main-type-dependencies-for-components
branch
from
6ae485e to
cbab06d
Compare
noahtallen
removed request for
ellatrix,
fabiankaegy,
juanmaguitar,
ndiego,
nerrad,
ntwb and
ryanwelcher
Member
Author
|
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
The reason I say they may not impact 3rd party repos is because we haven't seen those errors in our installation of |
tyxla
approved these changes
Sep 29, 2023
Member
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
Let's proceed, and if we encounter edge case errors, we can always do a followup.
Thanks @noahtallen 🚀
noahtallen
force-pushed
the
add-main-type-dependencies-for-components
branch
from
cbab06d to
a52f974
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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?
This partially addresses #50157. It moves some
@typesdependencies to the main dep field of@wordpress/components.Why?
When publishing a package, dev dependencies are never installed by 3rd party consumers. However, when we import a type from an external dep in our own published types, that external dep must be installable by our 3rd party consumers, or they'll get errors in
tscabout missing packages. As a result, those@types/packages we reference in our published build types must be normal dependencies in the specific package, rather than dev dependencies in the root package.json.(The approach of adding all
@typespackages to the root package.json only works when we only use our type info internally -- we'll need to adjust this approach now that we're publishing our own type information more frequently.)How?
See above
Testing Instructions
CI
Testing Instructions for Keyboard
Screenshots or screencast