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

Upgrade @types/react package and @types/react-dom #60796

Merged
merged 5 commits into from
May 8, 2024

Conversation

mrmurphy
Copy link
Contributor

What?

Upgrades the @types/react and @types/react-dom packages

Why?

We have a project depending on @wordpress/components that upgraded @types/react to the latest version. When that happened we ran into the build error mentioned here:

DefinitelyTyped/DefinitelyTyped#69006

How?

I've just updated the versions in the dependencies and addressed the two compiler errors that the upgrade produced.

  1. Remove "placeholder" from border-control component. As far as I could tell, that attribute wasn't available on the underlying type and wasn't documented or useful to the BorderControl component.
  2. Cast View to a specific type. This one I'm less sure about. The compiler wasn't happy when trying to assign the result of styled.div`` to the specified type. Here was the error:
Type 'StyledComponent<{ theme?: Theme | undefined; as?: ElementType<any, keyof IntrinsicElements> | undefined; }, DetailedHTMLProps<HTMLAttributes<HTMLDivElement>, HTMLDivElement>, {}>' is not assignable to type 'WordPressComponent<"div", ViewProps & RefAttributes<any>, true>'.
  Type 'ReactNode' is not assignable to type 'Element | null'.
    Type 'undefined' is not assignable to type 'Element | null'.ts(2322)

I addressed this by casting the result of styled.div to the specific desired type. I'm not sure how safe this is, but I think TS would warn me if that cast weren't compatible, like it does if I try to cast a string literal to a number.

@mrmurphy mrmurphy requested a review from ajitbohra as a code owner April 16, 2024 20:48
Copy link

github-actions bot commented Apr 16, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @mrmurphy.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: mrmurphy.

Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: sirreal <jonsurrell@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @mrmurphy! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Apr 16, 2024
@mirka mirka added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Apr 17, 2024
@mirka mirka self-requested a review April 17, 2024 12:04
@sirreal sirreal self-requested a review April 17, 2024 17:46
Copy link
Member

@mirka mirka left a comment

Choose a reason for hiding this comment

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

I've look into this, and both of the problems that are addressed here (BorderControl and View) are surfacing typing errors that should be fixed in a more complete way. And it looks like those are each important enough to warrant their own PRs and reviews.

Could you hold off a bit on this so I can fix the type errors separately first? I think I can do it next week.

Also, heads up that a TS update was merged recently (#60793), though it may not be specifically relevant to this problem.

@mrmurphy
Copy link
Contributor Author

Great, thank you @mirka! Do you want to close this PR and open a new one? Or just keep it open until next week?

@mirka
Copy link
Member

mirka commented Apr 22, 2024

Do you want to close this PR and open a new one?

Let's keep this one. I only wanted to separate out the type fixes.

I proposed a fix for View in #60919. The BorderControl problem was simpler than I thought and doesn't warrant a separate PR, so let's just add the fix in this PR:

diff --git a/packages/components/src/border-control/types.ts b/packages/components/src/border-control/types.ts
index b1ff87aaf5..5e028050d8 100644
--- a/packages/components/src/border-control/types.ts
+++ b/packages/components/src/border-control/types.ts
@@ -61,6 +61,10 @@ export type BorderControlProps = ColorProps &
 		 * interaction that selects or clears, border color, style, or width.
 		 */
 		onChange: ( value?: Border ) => void;
+		/**
+		 * Placeholder text for the number input.
+		 */
+		placeholder?: HTMLInputElement[ 'placeholder' ];
 		/**
 		 * An internal prop used to control the visibility of the dropdown.
 		 */

We should be good to go once the View fix is merged.

@mirka
Copy link
Member

mirka commented Apr 24, 2024

@mrmurphy The fix for View is merged now 👍

@sirreal
Copy link
Member

sirreal commented Apr 29, 2024

Is anything blocking this? @mirka are you planning to land it or do you need help? I think (hope) this will fix some intermittent type issues with react and ariakit like this:

node_modules/@ariakit/react-core/cjs/tooltip/tooltip-provider.d.ts:14:100 - error TS2694: Namespace '"…/node_modules/@types/react/jsx-runtime"' has no exported member 'JSX'.

14 export declare function TooltipProvider(props?: TooltipProviderProps): import("react/jsx-runtime").JSX.Element;

@youknowriad
Copy link
Contributor

Since this is becoming a blocker for me in another PR. I went ahead and applied the review suggestion. This should be ready to go.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

There are newer versions of these types now:

"@types/react": "18.3.1"
"@types/react-dom": "18.3.0"

It would be nice to get on the latest version.

I've tried updating that, but I'm having problems building locally.

I did notice that we have multiple versions of react types which is not ideal. There's a single version of react used and there should be a single set of types. react-native-safe-area seems to depend on v16 react types. That can (and should!) be solved with overrides, but there still seemed to be other incompatibilities between types when I tried to build locally.

packages/element/package.json Outdated Show resolved Hide resolved
@mirka
Copy link
Member

mirka commented May 7, 2024

No blockers on my end, please feel free to commandeer and merge since I will be unavailable for the next few days.

@youknowriad
Copy link
Contributor

I'm going to need some help here :) I'm having trouble reproducing the failures reported in CI locally.

@youknowriad
Copy link
Contributor

Oh I think the remaining issue is just because the compressed size check switches branches and because of the different versions of the package between the branches, it results in errors but the errors are not really there if you actually clean everything properly.

Based on that, I'm going to force merge this one. There's probably something to be done to "fix" that job but since it's an external action, it's hard to change. Maybe we can do something in our cleanup function (types linger)

@youknowriad youknowriad merged commit 966f0a1 into WordPress:trunk May 8, 2024
59 of 61 checks passed
Copy link

github-actions bot commented May 8, 2024

Congratulations on your first merged pull request, @mrmurphy! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 18.4 milestone May 8, 2024
@sirreal
Copy link
Member

sirreal commented May 8, 2024

I'm worried this will still cause problems, try checking out trunk and running:

npm ci
./node_modules/.bin/tsc --build --force

@youknowriad are you able to reproduce build issues like that? I'll try to help fix this.

@youknowriad
Copy link
Contributor

I have no build issues in trunk (using the instructions above) and the CI jobs in trunk are passing.

@oandregal
Copy link
Member

Trunk no longer works for me doing npm install && npm run dev:

Captura de ecrã 2024-05-08, às 12 50 16

@youknowriad
Copy link
Contributor

@oandregal can you try after cleaning everything?

rm -rf build
rm -rf packages/*/build*
npm run clean:package-types

@oandregal
Copy link
Member

can you try after cleaning everything?

It works after doing that. 👍

mcsf added a commit that referenced this pull request May 8, 2024
Following the merge of #60796, developers may face build issues that
require package types to be rebuilt. The problem is that `tsc --build`
fails somewhat silently -- or rather, there is some output, but it's not
clear in the console which stage of the `build:package-types` command
failed, and hence what the workaround should be.

This commit alleviates the issue by logging a helpful message in the
console if `tsc --build` fails:

    tsc failed. Try cleaning up first: `npm run clean:package-types`
mcsf added a commit that referenced this pull request May 8, 2024
Following the merge of #60796, developers may face build issues that
require package types to be rebuilt. The problem is that `tsc --build`
fails somewhat silently -- or rather, there is some output, but it's not
clear in the console which stage of the `build:package-types` command
failed, and hence what the workaround should be.

This commit alleviates the issue by logging a helpful message in the
console if `tsc --build` fails:

    tsc failed. Try cleaning up first: `npm run clean:package-types`
sirreal added a commit that referenced this pull request May 8, 2024
Squashed commit of the following:

commit 40c0fd7
Author: Jon Surrell <sirreal@users.noreply.github.com>
Date:   Wed May 8 18:55:56 2024 +0200

    Patch react-autosize-textarea for updated types

commit 69cf754
Author: Jon Surrell <sirreal@users.noreply.github.com>
Date:   Wed May 8 18:48:37 2024 +0200

    upgrade framer-motion

commit b383044
Author: Jon Surrell <sirreal@users.noreply.github.com>
Date:   Wed May 8 18:34:44 2024 +0200

    Upgrade @use-gesture/react

commit fdf93ae
Author: Jon Surrell <sirreal@users.noreply.github.com>
Date:   Wed May 8 18:32:11 2024 +0200

    Go bonkers on the global vars

commit cc43b8c
Author: Jon Surrell <sirreal@users.noreply.github.com>
Date:   Wed May 8 16:46:39 2024 +0200

    Fix the process problem

commit 548145e
Author: Jon Surrell <sirreal@users.noreply.github.com>
Date:   Wed May 8 13:33:23 2024 +0200

    No more skipLibCheck

commit 1eb7644
Author: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
Date:   Wed May 8 16:33:10 2024 +0100

    build: Suggest workaround if `tsc --build` fails (#61501)

    Following the merge of #60796, developers may face build issues that
    require package types to be rebuilt. The problem is that `tsc --build`
    fails somewhat silently -- or rather, there is some output, but it's not
    clear in the console which stage of the `build:package-types` command
    failed, and hence what the workaround should be.

    This commit alleviates the issue by logging a helpful message in the
    console if `tsc --build` fails:

        tsc failed. Try cleaning up first: `npm run clean:package-types`

commit 14ecb1d
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Wed May 8 09:49:20 2024 -0400

    Bump actions/checkout from 4.1.4 to 4.1.5 in the github-actions group (#61449)

    Bumps the github-actions group with 1 update: [actions/checkout](https://github.com/actions/checkout).

    Updates `actions/checkout` from 4.1.4 to 4.1.5
    - [Release notes](https://github.com/actions/checkout/releases)
    - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md)
    - [Commits](actions/checkout@0ad4b8f...44c2b7a)

    ---
    updated-dependencies:
    - dependency-name: actions/checkout
      dependency-type: direct:production
      update-type: version-update:semver-patch
      dependency-group: github-actions
    ...

    Signed-off-by: dependabot[bot] <support@github.com>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
    Co-authored-by: desrosj <desrosj@git.wordpress.org>

commit e899475
Author: Marin Atanasov <8436925+tyxla@users.noreply.github.com>
Date:   Wed May 8 16:48:50 2024 +0300

    RadioControl: Fix shrinking radio controls (#61476)

    Co-authored-by: tyxla <tyxla@git.wordpress.org>
    Co-authored-by: jameskoster <jameskoster@git.wordpress.org>

commit 839c17d
Author: Riad Benguella <benguella@gmail.com>
Date:   Wed May 8 14:39:34 2024 +0100

    Editor: Unify Header component. (#61273)

    Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
    Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
    Co-authored-by: jeryj <jeryj@git.wordpress.org>
    Co-authored-by: jasmussen <joen@git.wordpress.org>

commit 2c2f899
Author: Ella <4710635+ellatrix@users.noreply.github.com>
Date:   Wed May 8 20:30:44 2024 +0900

    Revert "useBlockSync: remove isControlled effect" (#61480)

    Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
    Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
@mrmurphy
Copy link
Contributor Author

mrmurphy commented May 8, 2024

Ahh thank you so much to all of you for wrapping this up. I moved on to other projects and hadn't circled back around. I really appreciate it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants