Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused type packages #75413

Merged
merged 3 commits into from
Apr 7, 2023
Merged

Remove unused type packages #75413

merged 3 commits into from
Apr 7, 2023

Conversation

noahtallen
Copy link
Contributor

Related to #74540

Proposed Changes

As part of work on Typescript and types with packages, I've been getting into a rabbit hole related to WordPress type definitions. This PR removes three DT packages since these types are provided by the packages themselves now.

Testing Instructions

TSC

@noahtallen noahtallen requested a review from a team April 6, 2023 19:01
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 6, 2023
@noahtallen noahtallen self-assigned this Apr 6, 2023
@github-actions
Copy link

github-actions bot commented Apr 6, 2023

@noahtallen
Copy link
Contributor Author

Hm, here's the error:

node_modules/@types/wordpress__data-controls/index.d.ts(8,10): error TS2305: Module '"@wordpress/data"' has no exported member 'Action'.

It must be trying to resolve types from the newer wp data package version which it doesn't actually support.

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@@ -293,6 +290,7 @@
"@types/wordpress__editor": "npm:[email protected]",
"@types/wordpress__notices": "npm:[email protected]",
"@types/wordpress__rich-text": "npm:[email protected]",
"@types/wordpress__data-controls/@wordpress/data": "npm:[email protected]",
Copy link
Contributor Author

@noahtallen noahtallen Apr 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok because @wordpress/data is only a dev dependency of the type package. Unfortunately DT doesn't allow dev dependencies, and using @wordpress/data for the test file is important. However, the dependency doesn't impact the actual type definitions. (I know this because I had to updated the data-controls package yesterday, after removing the DT definitions for @wordpress/data)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'd love to see all those hacks removed once all packages expose their own types.

@@ -166,12 +166,8 @@
"@types/uuid": "^8.3.4",
"@types/validator": "^13.7.1",
"@types/webpack-env": "^1.16.3",
"@types/wordpress__api-fetch": "^3.2.4",
"@types/wordpress__block-library": "^2.6.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block-library isn't used in calypso. While the other packages are, they bundle their own types (for quite a while actually)

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice cleanup, thanks 🚀

@@ -293,6 +290,7 @@
"@types/wordpress__editor": "npm:[email protected]",
"@types/wordpress__notices": "npm:[email protected]",
"@types/wordpress__rich-text": "npm:[email protected]",
"@types/wordpress__data-controls/@wordpress/data": "npm:[email protected]",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'd love to see all those hacks removed once all packages expose their own types.

@noahtallen noahtallen merged commit c3b50c0 into trunk Apr 7, 2023
@noahtallen noahtallen deleted the remove-unused-type-packages branch April 7, 2023 19:03
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants