-
Notifications
You must be signed in to change notification settings - Fork 384
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 outdated packages; Bump React to v18 #7548
Conversation
- Remove unsage of `render` from `@wordpress/element` - Use `act` from `@testing-library/react` instead of `react-dom/test-utils` - Update test cases for AMPDocumentStatusNotification component - Update test cases for AMPRevalidateNotification component - Update test cases for AMPValidationStatusNotification component - Update test cases for Icons component - Update test cases for SidebarNotification component - Update test cases for SidebarNotificationsContainer component - Update test cases for withAMPToolbarButton component - Update test cases for useAMPDocumentToggle hook - Update test cases for AMPToggle component - Update test cases for Nav component - Update test cases for ThemesContextProvider component - Update test cases for TemplateModeOption component - Update test cases for SiteScanSourcesList component - Update test cases for Selectable component - Update test cases for RedirectToggle component - Update test cases for ProgressBar component - Update test cases for PluginsContextProvider component - Update test cases for NavMenu component - Update test cases for Loading component - Update test cases for DevToolsToggle component - Update test cases for ConditionalDetails component - Update test cases for AMPNotice component - Update test cases for AMPSettingToggle component - Update test cases for AmpAdminNotice component - Update test cases for useValidationErrorStateUpdates hook - Update test cases for usePostDirtyStateChanges hook - Update test cases for Error component - Update test cases for CarouselNav component - Update test cases for useErrorsFetchingStateChanges hook
8dd1acf
to
42874b0
Compare
Plugin builds for eeebbd9 are ready 🛎️!
|
4b407be
to
9f7014a
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.
What an epic lift! Great work. Just a couple small comments, and aside from the failing tests, looks really good. (Granted, I'm not an expert on React!)
// container | ||
// .querySelector( | ||
// `.amp-error--${getErrorTypeClassName(status)} button` | ||
// ) | ||
// .click(); |
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.
Why is this commented-out code 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.
This code has been migrated to use the click event from @testing-library/react
and I missed removing it. Will remove it 👍🏼
createStore({ | ||
reviewLink: 'http://review-link.test', | ||
unreviewedValidationErrors: [ | ||
{ | ||
clientId: block.clientId, | ||
code: 'DISALLOWED_TAG', | ||
status: 3, | ||
term_id: 12, | ||
title: 'Invalid script: <code>jquery.js</code>', | ||
type: 'js_error', | ||
}, | ||
], | ||
validationErrors: [ | ||
{ | ||
clientId: block.clientId, | ||
code: 'DISALLOWED_TAG', | ||
status: 3, | ||
term_id: 12, | ||
title: 'Invalid script: <code>jquery.js</code>', | ||
type: 'js_error', | ||
}, | ||
], | ||
}); | ||
}); |
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 recall we chatted about this a bit, but I see why the change of status
to 1
? Why are unreviewedValidationErrors
and validationErrors
seemingly merged?
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.
Previously we were using registerStore
to create redux stores and then initializing them at required places. With this previously, we were able to initialize it with the initial state of our choice.
With the current implementation, we are initializing our store itself where it has been defined so there will be a single instance of store everywhere. We have combined both of them in one because it's the job of the reducer to decide which validation error will be assigned to unreviewed or reviewed as per
amp-wp/assets/src/block-validation/store/index.js
Lines 82 to 123 in 9f7014a
case SET_VALIDATION_ERRORS: | |
return { | |
...state, | |
ampCompatibilityBroken: Boolean( | |
action.validationErrors.filter( | |
({ status }) => | |
status === | |
VALIDATION_ERROR_NEW_REJECTED_STATUS || | |
status === | |
VALIDATION_ERROR_ACK_REJECTED_STATUS | |
)?.length | |
), | |
reviewedValidationErrors: | |
action.validationErrors.filter( | |
({ status }) => | |
status === | |
VALIDATION_ERROR_ACK_ACCEPTED_STATUS || | |
status === | |
VALIDATION_ERROR_ACK_REJECTED_STATUS | |
), | |
unreviewedValidationErrors: | |
action.validationErrors.filter( | |
({ status }) => | |
status === | |
VALIDATION_ERROR_NEW_ACCEPTED_STATUS || | |
status === | |
VALIDATION_ERROR_NEW_REJECTED_STATUS | |
), | |
keptMarkupValidationErrors: | |
action.validationErrors.filter( | |
({ status }) => | |
status === | |
VALIDATION_ERROR_NEW_REJECTED_STATUS || | |
status === | |
VALIDATION_ERROR_ACK_REJECTED_STATUS | |
), | |
validationErrors: action.validationErrors, | |
}; |
Previously, unreviewedValidationErrors
was being passed manually(which shouldn't be the case) to check if we are getting the expected results. By passing 1 as status the reducer automatically adds the validation error to the unreviewedValidationErrors
state.
Also, see how Gutenberg creates stores: https://github.com/WordPress/gutenberg/blob/trunk/packages/notices/src/store/index.js
@@ -331,6 +331,14 @@ jobs: | |||
env: | |||
COMPOSE_INTERACTIVE_NO_CLI: true | |||
|
|||
|
|||
# TODO: Remove it once node version in .nvmrc is updated to >=16. |
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.
Wait, why isn't .nvmrc
updated to node 18 if the engines.node
was updated in package.json
?
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.
Aah, I missed reverting the changes in engines
. Since many deps in @wordpress/*
are outdated, let's keep node to 14.
a9734e4
to
c9a285f
Compare
e20fd2a
to
146e4a3
Compare
146e4a3
to
eeebbd9
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.
🎉
QA Passed ✅
|
Summary
Previously we attempted to upgrade React in #7394 but due to React version inconsistencies on the upstream, we ended up blocking that PR.
This PR aims to upgrade React to v18 with the following enhancements:
@testing-library/react
.AfterActivationSiteScan
service to work with React >= 18.registerStore
in the@wordpress/data
package.Checklist