Update to typescript 5.1 #74540
Update to typescript 5.1 #74540
Conversation
matticbot
added
the
[Status] Needs Review
noahtallen
force-pushed
the
update-to-typescript-5
branch
from
8feb4c9
to
2ad507b
Compare
There was a problem hiding this comment.
Thanks for starting work on this 👍
While I haven't thoroughly read through the changelog, I'm assuming that the new version could have a bunch of fixes which make tests more precise.
One example I noticed is the group of errors related to unsupported props. I think that error actually makes sense and here's an example how I made it work locally:
diff --git a/packages/components/src/dialog/button-bar.tsx b/packages/components/src/dialog/button-bar.tsx
index 22b852296c..777cf2f166 100644
--- a/packages/components/src/dialog/button-bar.tsx
+++ b/packages/components/src/dialog/button-bar.tsx
@@ -1,7 +1,7 @@
import classnames from 'classnames';
import { isValidElement, cloneElement } from 'react';
import Button from '../button';
-import type { ReactElement, ReactNode, FunctionComponent } from 'react';
+import type { ReactNode, FunctionComponent } from 'react';
export type BaseButton = {
action: string;
@@ -15,10 +15,8 @@ export type BaseButton = {
target?: string;
};
-export type Button = ReactElement | BaseButton;
-
type Props = {
- buttons?: Button[];
+ buttons?: BaseButton[];
baseClassName: string;
onButtonClick: ( button: BaseButton ) => void;
};
diff --git a/packages/components/src/dialog/index.tsx b/packages/components/src/dialog/index.tsx
index 819dc4a221..a62ae3f88b 100644
--- a/packages/components/src/dialog/index.tsx
+++ b/packages/components/src/dialog/index.tsx
@@ -3,7 +3,7 @@ import { useCallback } from 'react';
import Modal from 'react-modal';
import Gridicon from '../gridicon';
import ButtonBar from './button-bar';
-import type { Button, BaseButton } from './button-bar';
+import type { BaseButton } from './button-bar';
import type { PropsWithChildren } from 'react';
import './style.scss';
@@ -12,7 +12,7 @@ type Props = {
additionalClassNames?: Parameters< typeof classnames >[ 0 ];
additionalOverlayClassNames?: Parameters< typeof classnames >[ 0 ];
baseClassName?: string;
- buttons?: Button[];
+ buttons?: BaseButton[];
className?: string;
isBackdropVisible?: boolean;
isFullScreen?: boolean;
That being said, I wonder if there was a good reason it was typed that way before 🤔
Those new "Cannot find module '@automattic/components' or its corresponding type declarations." errors are also interesting. Any idea what changed with regards to module resolution?
I think this happens when there's a type issue compiling an internal dependency. Since other packages depend on it, they now fail since the dependency on components fails. Thanks for looking into that; I'll try to integrate it! |
|
I like your fix for the buttons! I think the reason we allowed the wp-calypso/packages/components/src/dialog/button-bar.tsx Lines 36 to 38 in b96434e But it looks like just using |
noahtallen
force-pushed
the
update-to-typescript-5
branch
from
5feb430
to
430c7f2
Compare
| @@ -40,7 +40,7 @@ export const ListTile = ( { | |||
| typeof trailing === 'string' ? ( | |||
| <div className="list-tile__trailing">{ trailing }</div> | |||
| ) : isValidElement( trailing ) ? ( | |||
| cloneElement( trailing, { | |||
| cloneElement( trailing as React.ReactElement, { | |||
There was a problem hiding this comment.
From what I can tell, this is still correct -- leading can be either ReactNode or ReactElement. The isValidElement check means we know it's an element here, which means we can access its props directly below.
Though, this seems to indicate that the type restriction from isValidElement isn't working out of the box any more 🤔
There was a problem hiding this comment.
The trouble here is that TS doesn't know if className is a valid prop for the trailing element. isValidElement assures that it's of type ReactElement<unknown>, while asserting it as ReactElement makes it ReactElement<any>.
I'm confused by the six overloads of cloneElements type, but apparently the difference between unknown and any matters.
I was able to fix the type error by calling:
isValidElement< { className: string } >( trailing )Ideally we should specify a more specific type on leading and trailing, saying that it must be something on what we can put a className prop.
There was a problem hiding this comment.
Added the < { className: string } > fix
There was a problem hiding this comment.
Added this in one other place, but I noticed another issue where isValidElement won't narrow to ReactElement whatsoever, even when we don't care about the type of the props. See microsoft/TypeScript#53178 (comment)
In that case, it was easy to fix with our own wrapper function:
function isElement( el ): el is ReactElement {
return isValidElement( el );
}This was the root cause of issues with some base button types where you could pass either a react element or an object
| @@ -3,7 +3,7 @@ import wpcomRequest from 'wpcom-proxy-request'; | |||
| import type { GlobalStylesObject } from '../types'; | |||
|
|
|||
| const useGetGlobalStylesBaseConfig = ( siteId: number | string, stylesheet: string ) => { | |||
| return useQuery< any, unknown, unknown >( | |||
| return useQuery( | |||
There was a problem hiding this comment.
This was preventing type inference from working -- the type parameter to wpcomRequest below was actually enough to get the return type inferred correctly. (But that didn't work with any/unknown type params to useQuery)
packages/block-renderer/src/components/block-renderer-container.tsx
Outdated
Show resolved
Hide resolved
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~386 bytes added 📈 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~352 bytes added 📈 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~383 bytes added 📈 [gzipped]) React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
noahtallen
force-pushed
the
update-to-typescript-5
branch
2 times, most recently
from
798a8a8
to
49134c1
Compare
|
This PR modifies the release build for editing-toolkit To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-mMA-p2 |
|
Pretty much everything is fixed now, but there are some issues with internal WordPress data and data control types when compiling ETK. I don't understand why those aren't problematic for the other packages, but they do seem to be resolved when I temporarily update wp data to a newer version. (So this might be blocked on React 18.) Unless this is an internal problem, I think we may need to update the DT definitions for WP data controls as well. |
I'd also expect that most of the data controls usage could be refactored to simpler means. The generators are no longer in use in Gutenberg and we should likely follow suite. |
noahtallen
force-pushed
the
update-to-typescript-5
branch
from
304c678
to
af6e213
Compare
|
Rebased and updated to Typescript 5.1. Will then get this finished after #78711! |
| beginning!: number; | ||
| end?: number; | ||
| data: ReportData = new Map(); | ||
| startCollectors!: Collector[]; | ||
| stopCollectors!: Collector[]; | ||
|
|
||
| // Stubs that are necessary to implement but are not used. | ||
| body = null; |
There was a problem hiding this comment.
Typescript's definitions now require us to implement these, but we don't seem to use them anywhere.
noahtallen
force-pushed
the
update-to-typescript-5
branch
3 times, most recently
from
5d2f349
to
a07b296
Compare
|
I've triggered Pre-Release Tests against the calypso.live image and it passed. |
* Fix the GlobalStylesContext type
- Use wrapper fn to handle issues with isValidElement
- This applies to createSelector and treeSelect
noahtallen
force-pushed
the
update-to-typescript-5
branch
from
4a241fc
to
881de10
Compare
github-actions
bot
removed
the
[Status] Needs Review
Proposed Changes
This PR starts the update to Typescript 5, which was just released. I mostly wanted to see how much extra work this would require (if any). So far, a couple issues have come up, which we can track here.
See also: WordPress/gutenberg#52621
Testing Instructions
To Do:
treeSelectandcreateSelector. This was hard to track down, but it might be a TS bug: JS generic inference change after upgrading to 5.1 microsoft/TypeScript#55192. This was fixed by duplicating the generics which had multiple candidates, extending from the generic we want TS to pick.cloneElementandisValidElementhave some new issues where types won't narrow in the same way as before if no generics are passed to props.ButtonBarcomponent. Typescript doesn't think thatReactElementincludes theclassNameproperty, along with several other properties. This was caused by an issue withisValidElementnot narrowing types correctly: 5.0: Narrowing from type-guards ignored when constructing JSX call (2604) microsoft/TypeScript#53178 (comment)useContextcalls which rely on aGlobalStylesContextimport from@wordpress/edit-site. It now thinks the type isunknown, but previously it didn't complain about any issues here. (Example issue) These global styles issues were fixed by that team.@wordpress/data's build types cause errors in ETK. Appears to be resolved when updating to newer wordpress data versions. (Potentially blocking the PR on React 18)@wordpress/data-control's types cause errors in ETK. This might require a DT update. (See here)