Replace legacy context API with the new React.createContext, with a fallback #427

errendir · 2018-10-23T19:13:47Z

Hello

I have recently stumbled upon a very strange bug related to the use of the React legacy context API: facebook/react#13920

React will sometimes ignore shouldComponentUpdate returning false if legacy context is involved. This can lead to unnecessary rerendering. It's possible that the bug won't be fixed in React, since legacy context API is being phased out anyway.

This PR replaces the use of the legacy context API in react-transition-group with the new API (React.createContext) and a polyfill. Polyfilling is necessary if we want to support older versions of React. I used create-react-context polyfill, please let me know if there if you'd prefer a different one.

The Transition component is now wrapped with the added withTransitionGroup HOC. This makes it impossible to get the ref to the old Transition component. This can be fixed with React.forwardRef, but this feature is not available (and cannot be polyfilled AFAIK) in the older versions of React. I don't think instances of the Transition component were ever part of the public API of react-transition-group, so I don't think this change will break any existing code. Please let me know if this is a problem.

Some tests needed adjustment, since the actual stateful Transition component is not on the top of the tree of the rendered instances.

I tested my changes against React 16.5.2 and 15.6.2. Testing against 15.6.2 can be done by replacing:

"react": "^16.5.2",
"react-dom": "^16.5.2",
"react-test-renderer": "^16.5.2",

with

"react": "^15.6.2",
"react-dom": "^15.6.2",
"react-test-renderer": "^15.6.2",
"enzyme-adapter-react-15": "^1.1.1",

in package.json and replacing:

const Adapter = require('enzyme-adapter-react-16');

with

const Adapter = require('enzyme-adapter-react-15');

in test/setup.js

…lyfill

jquense · 2018-10-23T19:19:04Z

package.json

@@ -54,6 +54,7 @@
    "react-dom": ">=15.0.0"
  },
  "dependencies": {
+    "create-react-context": "^0.2.3",


i don't really want to depend on CRC, it's a biggish polyfill and folks don't like the license

i'm not sure if it's worth doing a major bump yet for this either :/

Perhpas we could use/write a smaller polyfill. Alternatively a separate @next package could be published, dropping support for older React.

The context API update will have to happen eventually :)

errendir · 2018-10-23T19:41:37Z

@jquense I replaced create-react-context with a custom code, that fallsback to legacy context if React.createContext is missing. It's as similar to what was there before as possible.

jquense · 2018-10-25T13:09:07Z

@errendir I think it's actually fine at this point to bump to react 16.6 as the peer minimum. Lets just use the "native" api with no fallback

jquense · 2018-10-25T13:11:40Z

src/TransitionGroupContext.js

+  render() { return this.props.children }
+}
+
+export function withTransitionGroup(ComponentToWrap) {


since we are gonna use just the React api's, lets remove this, and go for the new contextType api in Transition maybe? Alternatively keep the HoC but lets use forwardRef as well

Actually for a HoC lets use react-context-toolbox's mapContextToProps helper which takes care of this

OK, done. See the comment on the PR about some issues with the "react-context-toolbox" module.

@jquense Do you still want to go with react-context-toolbox package? contextType looks the best way. Why HOC?

jquense · 2018-10-25T13:12:24Z

test/Transition-test.js

@@ -20,6 +20,10 @@ jasmine.addMatchers({
  }),
 })

+function findStatefulInstance(wrapper) {
+  return wrapper.find(Transition.__TestingStatefulComponent).instance()


wrapper.find('Transtition') should be fine and avoids the extra static prop

Good idea! I wasn't sure if there weren't two components with the displayName "Transition" (some HOC perhaps), but it does work. PR updated

…e fallback and require peerDependency react >=16.3.0. Simplify tests.

errendir · 2018-10-25T16:08:53Z

@jquense I updated the PR to use "react-context-toolbox", but I think there are some packaging issues with that lib. For one thing it has paths to ES2015 modules in its package.json (https://github.com/4Catalyzer/react-context-toolbox/blob/master/package.json#L5), but those files are not actually present in the npm module. To see that run:

npm pack react-context-toolbox@1.2.2
tar xvzf react-context-toolbox-1.2.2.tgz
ls package

This triggers the linter errors. The second issue is that index.js uses a named import (https://github.com/4Catalyzer/react-context-toolbox/blob/master/src/index.js#L1), but mapContextToProps is exported as a default: https://github.com/4Catalyzer/react-context-toolbox/blob/master/src/mapContextToProps.js#L57

I worked around this by importing directly from "react-context-toolbox/lib/mapContextToProps", but we should fix this.

I also bumped react peerDependency to 16.3.0. This is the release that includes the new context and forwardRef and the tests pass on that version. Will we need to bump up the major semver number because of this change?

TrySound · 2019-01-15T08:33:27Z

test/Transition-test.js

@@ -264,7 +268,7 @@ describe('Transition', () => {

        return (
          <Transition
-            ref="transition"
+            ref={transition => this.transition = this.transition || transition}


@errendir Could you create PR with only ref upgrade in tests? It's unrelated and can be merged faster.

OK, I removed the ref changes from this PR. I also solved merge conflicts with master branch. Please let me know if there is anything else I can do to get this PR merged.

I will create a separate PR for the ref changes.

…d-compatible * origin/master: chore: bundle UMD with rollup (reactjs#449) chore(release): 2.5.3 [skip ci] fix: strip custom prop-types in production (reactjs#448) chore: fix storybook (reactjs#445) chore(release): 2.5.2 [skip ci] fix: pass appear to CSSTransition callbacks (reactjs#441) docs: Fix type declaration comment (reactjs#439) chore(release): 2.5.1 [skip ci] fix: prevent calling setState in TransitionGroup if it has been unmounted (reactjs#435)

Replace legacy context API with the new React.createContext with a po…

d72883d

…lyfill

jquense reviewed Oct 23, 2018

View reviewed changes

Don't use create-react-context

e4332d0

errendir changed the title ~~Replace legacy context API with the new React.createContext with a polyfill~~ Replace legacy context API with the new React.createContext, with a fallback Oct 23, 2018

jquense reviewed Oct 25, 2018

View reviewed changes

Use react-context-toolbox for passing context value into props. Remov…

320b0e5

…e fallback and require peerDependency react >=16.3.0. Simplify tests.

TrySound reviewed Jan 15, 2019

View reviewed changes

errendir added 2 commits February 13, 2019 19:21

Undo the ref change

aafaecd

errendir force-pushed the new-context-backward-compatible branch from 873ba5b to ab6d376 Compare February 14, 2019 03:51

errendir mentioned this pull request Feb 14, 2019

Don't use string refs in tests #456

Merged

TrySound mentioned this pull request Mar 12, 2019

breaking: Use stable context API #471

Merged

jquense closed this Apr 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Replace legacy context API with the new React.createContext, with a fallback #427

Replace legacy context API with the new React.createContext, with a fallback #427

Uh oh!

errendir commented Oct 23, 2018

Uh oh!

jquense Oct 23, 2018

Uh oh!

jquense Oct 23, 2018

Uh oh!

errendir Oct 23, 2018

Uh oh!

errendir commented Oct 23, 2018

Uh oh!

jquense commented Oct 25, 2018

Uh oh!

jquense Oct 25, 2018

Uh oh!

jquense Oct 25, 2018

Uh oh!

errendir Oct 25, 2018

Uh oh!

TrySound Feb 15, 2019

Uh oh!

jquense Oct 25, 2018

Uh oh!

errendir Oct 25, 2018

Uh oh!

errendir commented Oct 25, 2018

Uh oh!

TrySound Jan 15, 2019

Uh oh!

errendir Feb 14, 2019

Uh oh!

Uh oh!

Replace legacy context API with the new React.createContext, with a fallback #427

Replace legacy context API with the new React.createContext, with a fallback #427

Uh oh!

Conversation

errendir commented Oct 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errendir commented Oct 23, 2018

Uh oh!

jquense commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

errendir commented Oct 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!