-
Notifications
You must be signed in to change notification settings - Fork 144
Update dependencies to support react 17 #8305
Conversation
1dcab4e
to
fb9dd72
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Appreciate this very laborious and often thankless work.
Automated tests and smoke testing went well, almost all the changes look fine.
I went over the changelogs of every package that was updated and the only things that stood out were:
- Most of them had breaking changes of IE11 deprecation and Node.JS LTS 12 as the minimum version - wonder if we should add this to our own changelog?
- @wordpress/components deprecated some props (isPrimary, isSecondary, isTertiary, isLink) in
<Button>
and we use those props, so we should probably make a separate issue to fix them before they completely remove support. https://github.com/WordPress/gutenberg/blob/81e8e0f7c999df3cc43ef97cb2913779c0190eff/packages/components/CHANGELOG.md#deprecation-1
@rjchow Thanks for spotting them. I've add IE11 deprecation note to changelogs in bd4a659. I think our minimum Node JS version is already 12 as defined in the package.json. Also created a follow-up issue #8313 for |
fc46931
to
44ec66c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for the work here, @chihsuan! It's super neat that you've also properly separated the changes into different commits, definitely helped with reviewing there.
I've found an issue with createNotice
changes. Everything else is looking good so far!
@@ -69,7 +69,7 @@ class ReviewsPanel extends Component { | |||
actions: [ | |||
{ | |||
label: __( 'Undo', 'woocommerce-admin' ), | |||
onClick: () => { | |||
callback: () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated createNotice actions param onclick (deprecated) -> callback. 4666b8f
I've tested this change and found out the new callback didn't get triggered. I've tried to change it back to onClick
and it worked. Also, looking at the source for core/notices
, I'm seeing onClick
is still used (at least in the typedef) and no reference to callback
is found. Kinda weird, where did you find the deprecated notice for onClick
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.npmjs.com/package/@types/wordpress__notices
The last published is a year ago. : (
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me drop this commit 4666b8f and possibly fill an issue on Gutenberg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted the changes and confirmed they actually changed from callback
to onClick
(WordPress/gutenberg#17206), but didn't update the file in DefinitelyTyped repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see. Yeah it seems that'll need an update
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I've raised a discussion DefinitelyTyped/DefinitelyTyped#58850 based on DefinitelyTyped/DefinitelyTyped#53377.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! It seems that the original owner for the type module was an a11n, but no longer works here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tagged the other maintainers from @types/wordpress__components
🥲
c2fc69a
to
9d9ce64
Compare
Update pkg Update @wordpress/api-fetch and gridicons
wp-components has included it
Update changelogs
Update webpack config comment Fix grammar
9d9ce64
to
074b51f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm that createNotice
is working again. LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smoke test went well for me! Nice job on this @chihsuan 👍
…n#8305) * Upgrade dependencies to support react17 Update pkg Update @wordpress/api-fetch and gridicons * Update test snapshots * Update SnackbarList with latest react-spring * Mock data.dispatch for task-list-item.test.tsx * Remove '@wordpress/components/src/visually-hidden/style' import wp-components has included it * Update interpolateComponents import path * Fix display-options test * Add changelogs Update changelogs * Add @automattic/explat-client-react-helpers back * Update webpack.config for explat-client-react-helpers Update webpack config comment Fix grammar
Fixes #8283
This PR updates the dependencies to support react 17 with the following changes.
Added-> Update webpack.config for latest published explat-client-react-helpers. c2fc69aexplat-client-react-helpers.tsx
fromwp-calypso/packages/explat-client-react-helpers
topacakges/explat
as temporary solution to support react 17. 22befc9SnackbarList
with latest react-spring. 37ca4a5'@wordpress/components/src/visually-hidden/style'
import (included in wp-components). 42a4ddeinterpolateComponents
import path ->'@automattic/interpolate-components'
18b60b9Updated-> Update dependencies to support react 17 #8305 (review)createNotice
actions paramonclick
(deprecated) ->callback
. 4666b8fDetails: p90Yrv-2Lk-p2
Detailed test instructions:
npm run clean && rm -rf node_modules && rm -rf packages/**/node_modules
npm install && npm run build
npm run test