Fix version conflicts for @types/react #24294

manzoorwanijk · 2022-05-09T11:21:28Z

Due to type conflict between v17 and v18 of @types/react, we have some TypeScript errors in certain components. For example on master branch, if you run cd projects/js-packages/components/ && pnpm tsc, you will see TS error(s).

Changes proposed in this Pull Request:

Override @types/react^17 peer dependency for @testing-library/react-hooks in order to use @types/react^18
Upgrade @types/react to v18

Jetpack product discussion

See #24256 (comment) and #24276 (comment)

Does this pull request change what data or activity we track or use?

No

Testing instructions:

Checkout the PR
Run jetpack install --all or pnpm install
Confirm that you don't seen any MISSING PEER DEPENDENCY error.
Run cd projects/js-packages/components/ && pnpm tsc
Confirm that you don't see any TS error

Next Steps

I think it's the time to make TS check a part of the build process by simply running the TypeScript compiler CLI - tsc before running the actual build commands to avoid any errors being left in TS components.
It will be as simple as adding a "build": "tsc && ..." script to js-packages/components and plugins/jetpack etc.
Thoughts @Automattic/jetpack-agora ?

…ooks

matticbot · 2022-05-09T11:21:36Z

Caution: This PR has changes that must be merged to WordPress.com
Hello manzoorwanijk! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D80354-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

github-actions · 2022-05-09T11:22:38Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
⚠️ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: June 7, 2022.
Scheduled code freeze: May 31, 2022.

anomiex · 2022-05-09T12:49:37Z

Override @types/react^17 peer dependency for @testing-library/react-hooks in order to use @types/react^18

Upgrade @types/react to v18

I note we're still using react v17, not v18. Should the types package's version match the react version?

I think it's the time to make TS check a part of the build process by simply running the TypeScript compiler CLI - tsc before running the actual build commands to avoid any errors being left in TS components.
It will be as simple as adding a "build": "tsc && ..." script to js-packages/components and plugins/jetpack etc.

What impact would that have on build time? Could the actual build process report errors instead of having to run a separate build before the build?

manzoorwanijk · 2022-05-09T13:32:54Z

I note we're still using react v17, not v18. Should the types package's version match the react version?

That may not make a huge difference in terms of types. The reason for my suggestion to use v18 is that there are already some underlying packages that already have v18 as a dependency. if you search for '@types/react' in the lockfile, you will see the number of packages that are already using v18.

What impact would that have on build time?

Yes, just a little bit.

Could the actual build process report errors instead of having to run a separate build before the build?

Yes, for that we need to wire up a TS checker plugin with webpack which won't be any different than tsc and instead is a little slower than tsc.

anomiex · 2022-05-09T13:58:44Z

I note we're still using react v17, not v18. Should the types package's version match the react version?

That may not make a huge difference in terms of types. The reason for my suggestion to use v18 is that there are already some underlying packages that already have v18 as a dependency. if you search for '@types/react' in the lockfile, you will see the number of packages that are already using v18.

I see only four. All are other @types/* packages.

What impact would that have on build time?

Yes, just a little bit.

When you get around to trying it, let's be sure to quantify the impact. We might also try running tsc concurrently with the build instead of serially before it.

Some quick searches suggest that the tsconfig should have options like noEmit or emitDeclarationOnly so tsc doesn't write files.

Yes, for that we need to wire up a TS checker plugin with webpack which won't be any different than tsc and instead is a little slower than tsc.

Yeah, now that I look I see babel just understands the syntax well enough to ignore it, it doesn't actually process the types.

manzoorwanijk · 2022-05-09T14:22:40Z

I see only four. All are other @types/* packages.

It's possible that more external packages will upgrade soon.

When you get around to trying it, let's be sure to quantify the impact. We might also try running tsc concurrently with the build instead of serially before it.

Oh yeah, running it in concurrently is a great idea. Thank you.

manzoorwanijk · 2022-05-09T14:41:39Z

Some quick searches suggest that the tsconfig should have options like noEmit or emitDeclarationOnly so tsc doesn't write files.

Our tsconfig.base.json in js-tools already has noEmit set to true.

We can add emitDeclarationOnly for js-components to improve type experience.

anomiex · 2022-05-09T16:02:33Z

It's possible that more external packages will upgrade soon.

And when they do, will they also want react 18 so we'll have to wait on those updates anyway? That's why #24276 brought @testing-library/react to only 12.1.5 instead of 13.x, for example.

manzoorwanijk · 2022-05-09T16:38:30Z

And when they do, will they also want react 18 so we'll have to wait on those updates anyway?

Yeah we can do that. So, what's the easiest way to keep @types/react at v17 regardless of which package upgrades to v18?

…@types/react-version-conflict

anomiex · 2022-05-11T16:33:33Z

So, what's the easiest way to keep @types/react at v17 regardless of which package upgrades to v18?

For direct dependencies from our stuff, just don't do the upgrade.

If we also need to worry about indirect deps of things, I suppose if we really needed to we could see about using the afterAllResolved hook in pnpmfile to throw an error if v18 is getting added. After first cleaning up the existing four deps pulling it in.

manzoorwanijk · 2022-05-11T17:02:21Z

If we also need to worry about indirect deps of things, I suppose if we really needed to we could see about using the afterAllResolved hook in pnpmfile to throw an error if v18 is getting added. After first cleaning up the existing four deps pulling it in.

Do we have an example of how to do that? I have no idea 😃

anomiex · 2022-05-11T17:06:34Z

Do we have an example of how to do that? I have no idea smiley

Not that I know of, we've never done it before. The whole JS ecosystem is designed for entirely the opposite use case, that dependencies can have their own copies of any deps they need instead of having to be locked to a particular shared version.

…rsions

manzoorwanijk · 2022-05-13T09:17:26Z

@anomiex I have made the changes to use v17.

renatoagds

Working fine. LGTM

github-actions · 2022-05-17T15:43:14Z

Great news! One last step: head over to your WordPress.com diff, D80354-code, and deploy it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

manzoorwanijk · 2022-05-17T15:59:18Z

r245802-wpcom

anomiex · 2022-05-25T14:14:23Z

.pnpmfile.cjs

+	// Override @types/react* dependencies in order to use their specific versions
+	for ( const dep of [ '@types/react', '@types/react-dom', '@types/react-test-renderer' ] ) {
+		if ( pkg.dependencies?.[ dep ] ) {
+			pkg.dependencies[ dep ] = '17.x';


Just arbitrarily forcing every dep on these packages to "17.x" seems risky to me. But I guess we'll see what happens.

I was suggesting we avoid upgrading things that would bring in a 18.x dep rather than blindly overriding them.

manzoorwanijk added 2 commits May 9, 2022 16:36

Override @types/react^17 peer dependency for @testing-library/react-h…

654bfa6

…ooks

Upgrade @types/react to v18

5baefa5

manzoorwanijk added [Status] Needs Review This PR is ready for review. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [JS Package] Components labels May 9, 2022

manzoorwanijk requested review from a team and anomiex May 9, 2022 11:21

manzoorwanijk self-assigned this May 9, 2022

matticbot added Touches WP.com Files labels May 9, 2022

github-actions bot added the RNA label May 9, 2022

anomiex mentioned this pull request May 9, 2022

Update JS unit testing packages (major) #24276

Merged

1 task

kraftbj and others added 2 commits May 10, 2022 14:19

[not verified] Merge remote-tracking branch 'origin/master' into fix/…

771d588

…@types/react-version-conflict

Merge branch 'master' into fix/@types/react-version-conflict

e958550

manzoorwanijk mentioned this pull request May 11, 2022

Add TS check to build process #24329

Merged

manzoorwanijk added 3 commits May 13, 2022 14:41

Override @types/react* dependencies in order to use their specific ve…

3335e66

…rsions

Revert Upgrade @types/react to v18

13b3fbc

Merge branch 'master' into fix/@types/react-version-conflict

438f386

manzoorwanijk added 2 commits May 13, 2022 18:53

Merge branch 'master' into fix/@types/react-version-conflict

4680815

Merge branch 'master' into fix/@types/react-version-conflict

5e28755

renatoagds approved these changes May 17, 2022

View reviewed changes

manzoorwanijk merged commit e5e811e into master May 17, 2022

manzoorwanijk deleted the fix/@types/react-version-conflict branch May 17, 2022 15:41

github-actions bot added this to the jetpack/11.0 milestone May 17, 2022

github-actions bot removed the [Status] Needs Review This PR is ready for review. label May 17, 2022

anomiex reviewed May 25, 2022

View reviewed changes

Fix version conflicts for @types/react #24294

Fix version conflicts for @types/react #24294

Uh oh!

Conversation

manzoorwanijk commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this Pull Request:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

Next Steps

Uh oh!

matticbot commented May 9, 2022

Uh oh!

github-actions bot commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anomiex commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

manzoorwanijk commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anomiex commented May 9, 2022

Uh oh!

manzoorwanijk commented May 9, 2022

Uh oh!

manzoorwanijk commented May 9, 2022

Uh oh!

anomiex commented May 9, 2022

Uh oh!

manzoorwanijk commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anomiex commented May 11, 2022

Uh oh!

manzoorwanijk commented May 11, 2022

Uh oh!

anomiex commented May 11, 2022

Uh oh!

manzoorwanijk commented May 13, 2022

Uh oh!

renatoagds left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 17, 2022

Uh oh!

manzoorwanijk commented May 17, 2022

Uh oh!

anomiex May 25, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

manzoorwanijk commented May 9, 2022 •

edited

Loading

github-actions bot commented May 9, 2022 •

edited

Loading

anomiex commented May 9, 2022 •

edited

Loading

manzoorwanijk commented May 9, 2022 •

edited

Loading

manzoorwanijk commented May 9, 2022 •

edited

Loading