Upgrade React Redux and Redux Form #37438

jsnajdr · 2019-11-08T10:20:39Z

Renovate bot keeps force-pushing over my changes in #32129, so I'm creating a new PR with all the human interventions that are needed.

This will be a tough upgrade, as there are two breaking changes:

Refs to connected components:
connect used to require withRef: true flag if a ref was expected to be forwarded to the inner component. If it wasn't present, the ref would be assigned the outer, connected component.

In v7, the withRef flag was renamed to forwardRef. That's not hard to change. But there's additional gotcha: if forwardRef is not present, the ref passed to the outer component will be ignored. It will not be set to anything, not even the outer component. That can cause subtle issues that can be discovered only by manual testing.

I needed to change the following components:

Image Editor Canvas in media image editor. Ref is used to extract the edited image from the canvas element.
Zoninator extension autocomplete. Ref used to forward Up and Down key events inside search box to an autocomplete dropdown.
Notifications: scrolling the notification list as you move around with keyboard and open notifications. Here we have instances of the tricky cases with no withRef: things were silently failing.

No store in legacy React context:
At several places, we extract the store from legacy React context (defining contextTypes and using this.context. This no longer works, because store is now in new React context (with provider and consumer components).

The following places need to be updated:

the RootChild component that transfers the context to a different React mount tree
TinyMCE wrappers in Classic Editor and in Woo Store that forward the store to TinyMCE and its plugins
Guided Tours
Signup

Tests:
I also needed to update a few tests that used react-dom/test-utils, because they started failing for some reason with the new react-redux. I ended up rewriting them with Enzyme, its full mount capability and the easy-to-use component wrappers. With hindsight, all that was needed was probably wrapping the renders with act( ... ), but I think that the Enzyme rewrite is still definitely a clear step forward.

matticbot · 2019-11-08T10:20:41Z

Test live: https://calypso.live/?branch=upgrade/react-redux-and-form

matticbot · 2019-11-08T10:33:27Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~44 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest       +303 B  (+0.2%)      +44 B  (+0.1%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

App Entrypoints (~1050 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-login               +568 B  (+0.1%)     +343 B  (+0.1%)
entry-gutenboarding       +568 B  (+0.0%)     +342 B  (+0.1%)
entry-main                -118 B  (-0.0%)     +365 B  (+0.1%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~29547 bytes added 📈 [gzipped])

name                   parsed_size            gzip_size
post-editor               +51390 B   (+2.8%)   +14130 B   (+2.8%)
zoninator                 +51021 B  (+22.8%)   +15757 B  (+27.0%)
settings                    -211 B   (-0.0%)      -33 B   (-0.0%)
jetpack-connect             -187 B   (-0.0%)       +5 B   (+0.0%)
stats                        -98 B   (-0.0%)      -14 B   (-0.0%)
activity                     -98 B   (-0.0%)       -8 B   (-0.0%)
media                        -87 B   (-0.0%)      -33 B   (-0.0%)
gutenberg-editor             -87 B   (-0.0%)      -30 B   (-0.0%)
site-blocks                  -75 B   (-0.0%)      -25 B   (-0.0%)
security                     -75 B   (-0.0%)      -20 B   (-0.0%)
purchases                    -75 B   (-0.0%)      -27 B   (-0.0%)
privacy                      -75 B   (-0.0%)      -24 B   (-0.0%)
notification-settings        -75 B   (-0.0%)      -27 B   (-0.0%)
me                           -75 B   (-0.0%)      -24 B   (-0.0%)
help                         -75 B   (-0.0%)      -22 B   (-0.0%)
happychat                    -75 B   (-0.0%)      -23 B   (-0.0%)
account-close                -75 B   (-0.0%)      -23 B   (-0.0%)
account                      -75 B   (-0.0%)      -24 B   (-0.0%)
woocommerce                  -50 B   (-0.0%)      +20 B   (+0.0%)
people                       +47 B   (+0.0%)      +35 B   (+0.0%)
accept-invite                +47 B   (+0.0%)      -49 B   (-0.1%)
devdocs                      -34 B   (-0.0%)      +25 B   (+0.1%)
settings-writing             -24 B   (-0.0%)       -7 B   (-0.0%)
domains                      +24 B   (+0.0%)       +9 B   (+0.0%)
posts-pages                  -12 B   (-0.0%)      -11 B   (-0.0%)
posts-custom                 -12 B   (-0.0%)      -10 B   (-0.0%)

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 (~14828 bytes added 📈 [gzipped])

name                                     parsed_size           gzip_size
async-load-design-blocks                    +51257 B  (+1.9%)   +14844 B  (+2.4%)
async-load-design-playground                   -98 B  (-0.0%)       +4 B  (+0.0%)
async-load-design                              -98 B  (-0.0%)       +2 B  (+0.0%)
async-load-post-editor-media-modal             -87 B  (-0.0%)      -27 B  (-0.0%)
async-load-apps-notifications-index-jsx        +25 B  (+0.0%)       +5 B  (+0.0%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

sgomes · 2019-11-08T11:04:15Z

but I think that the Enzyme rewrite is still definitely a clear step forward.

Oh, I thought we were trying to drop Enzyme as a dependency and rewriting things with ReactDOM test utils wherever possible?

sgomes

Code-wise, this LGTM. Left some very minor comments. Will try to run some tests now.

sgomes · 2019-11-08T11:06:40Z

apps/notifications/src/panel/templates/index.jsx

@@ -1,12 +1,14 @@
 /**
- * @module templates/index
+ * External dependencies


Are the changes in this file related to this PR? I'm ok with keeping them, but I just want to make sure.

Some changes are ESLint fixes, this one related to missing internal/external docblock comments. And I also removed the @module docblock pragma -- we don't use it anywhere else.

sgomes · 2019-11-08T11:18:21Z

client/components/tinymce/index.jsx

+
+	render() {
+		return (
+			<ReactReduxContext.Consumer>


That's a clever, minimal fix, that doesn't entail too much rewriting 👍

sgomes · 2019-11-08T11:45:55Z

client/jetpack-connect/help-button.jsx

-export class JetpackConnectHelpButton extends PureComponent {
-	static propTypes = { label: PropTypes.string };
+export default function JetpackConnectHelpButton( { label } ) {
+	const dispatch = useDispatch();


sgomes

I played around with several things you touched in the live branch and couldn't find any issues 👍

The E2E tests should also add some measure of safety.

LGTM!

jsnajdr · 2019-11-08T12:51:23Z

Oh, I thought we were trying to drop Enzyme as a dependency and rewriting things with ReactDOM test utils wherever possible?

I'm not so sure about Enzyme tests that use the full React mount with JSDOM -- Enzyme's mount uses react-dom/test-utils internally and provides a convenient wrapper around it.

Some benefits of the Enzyme wrapper:

good API for finding components and elements inside the rendered tree (.find, .findWhere), as opposed to functions with silly names like scryRenderedDOMComponentsWithClass (originates from some internal FB libraries)
there is a library of Jest expect extensions in jest-enzyme that allows writing test assertions in a high-level and idiomatic way.

I don't want to go too deep into the React testing debate, as I have limited knowledge and no strong opinions about it. I mainly wanted to fix the tests that got broken during the react-redux upgrade. And also remove Chai and Sinon from some of the updated tests.

…e the component React Redux ref API has changed in 7.0.0. Modernization includes: - use modern refs - use arrow functions for bound methods - every variable has its own `const` decl

…nzyme option Instead of using the legacy React context, which is an implementation detail of particular React Redux version, use the Enzyme's `wrappingComponent` option to render a mock Redux provider above the tested component.

…omponent, use full mount React Redux 7.x no longer supports `store` prop for `Provider`, and uses the new `React.createContext()` instead of the legacy context. That requires an update of the unit tests. - use `wrappingComponent` option to activate a mock Redux provider - switch to full mount, as `wrappingComponent` doesn't work with `useContext` in Enzyme (enzymejs/enzyme#2176)

…ith useDispatch

sgomes · 2019-11-08T12:59:07Z

I don't want to go too deep into the React testing debate, as I have limited knowledge and no strong opinions about it. I mainly wanted to fix the tests that got broken during the react-redux debate. And also remove Chai and Sinon from some of the updated tests.

Sounds good to me 👍 Sorry if I derailed the conversation, but I was curious 🙂

jsnajdr added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Nov 8, 2019

jsnajdr requested review from ockham, sgomes, sirreal, diegohaz and a team November 8, 2019 10:20

jsnajdr self-assigned this Nov 8, 2019

sgomes reviewed Nov 8, 2019

View reviewed changes

sgomes approved these changes Nov 8, 2019

View reviewed changes

renovate-bot and others added 11 commits November 8, 2019 13:53

fix(deps): update dependency react-redux to v7

701470b

fix(deps): update dependency redux-form to v8

b185e80

ImageEditorCanvas: change react-redux withRef to forwardRef, moderniz…

72a4f2a

…e the component React Redux ref API has changed in 7.0.0. Modernization includes: - use modern refs - use arrow functions for bound methods - every variable has its own `const` decl

Zoninator Search Autocomplete: change react-redux withRef to forwardRef

2979823

Notifications: use react-redux forwardRef to get list element

1a5841a

Get Redux store from new React context in TinyMCE wrappers

b8ee8bb

Jetpack Connect Help Button: convert to hooked functional component w…

edb4cd9

…ith useDispatch

PluginsList: rewrite tests to use Enzyme

414c039

DomainManagement.List: rewrite tests to use Enzyme

9cd1aef

jsnajdr force-pushed the upgrade/react-redux-and-form branch from 53d88e1 to 9cd1aef Compare November 8, 2019 12:55

jsnajdr merged commit ba82ba2 into master Nov 12, 2019

jsnajdr deleted the upgrade/react-redux-and-form branch November 12, 2019 09:29

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 12, 2019

This was referenced Nov 13, 2019

Fix Popover regressions caused by react-redux upgrade #37553

Merged

SharingButtonsPreviewButton: add forwardRef flag to connect to make the button reffable #37570

Merged

pento mentioned this pull request Dec 10, 2019

Reader: scrolling problems in Reader #37901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade React Redux and Redux Form #37438

Upgrade React Redux and Redux Form #37438

jsnajdr commented Nov 8, 2019

matticbot commented Nov 8, 2019

matticbot commented Nov 8, 2019 •

edited

Loading

sgomes commented Nov 8, 2019

sgomes left a comment

sgomes Nov 8, 2019

jsnajdr Nov 8, 2019

sgomes Nov 8, 2019

sgomes Nov 8, 2019

sgomes left a comment

jsnajdr commented Nov 8, 2019 •

edited

Loading

sgomes commented Nov 8, 2019

Upgrade React Redux and Redux Form #37438

Upgrade React Redux and Redux Form #37438

Conversation

jsnajdr commented Nov 8, 2019

matticbot commented Nov 8, 2019

matticbot commented Nov 8, 2019 • edited Loading

sgomes commented Nov 8, 2019

sgomes left a comment

Choose a reason for hiding this comment

sgomes Nov 8, 2019

Choose a reason for hiding this comment

jsnajdr Nov 8, 2019

Choose a reason for hiding this comment

sgomes Nov 8, 2019

Choose a reason for hiding this comment

sgomes Nov 8, 2019

Choose a reason for hiding this comment

sgomes left a comment

Choose a reason for hiding this comment

jsnajdr commented Nov 8, 2019 • edited Loading

sgomes commented Nov 8, 2019

matticbot commented Nov 8, 2019 •

edited

Loading

jsnajdr commented Nov 8, 2019 •

edited

Loading