Adds react15CompatibilityMode setting #540

jimbolla · 2016-11-09T21:00:45Z

react15CompatibilityMode is intended to be a temporary setting until our official solution is "use React v16." When set to true, which is the default in <Provider>, subscriptions are not re-ordered to fire top-down. This avoids the bug related to #525 but loses some of the perf gains and re-introduces the issues with state and ownProps being out of sync. This should behave pretty much like react-redux v4.

A developer that upgrades to this version w/o making any other changes should not experience the #525 bug out of the box, but won't get the good stuff mentioned above.

The recommended upgrade path is to set react15CompatibilityMode to false at the <Provider> level and then set it to true at the component level for all connected text inputs. This can be done either as a prop (confirmed working with redux-form's <Field> component) or as an options arg to connect. Those inputs are now vulnerable to the state/props sync issue that exists in v4, but will not have the #525 cursor issue.

Apps using React v16 (or whatever version includes facebook/react#8204), or using Preact as a drop-in replacement for React should not have to set react15CompatibilityMode to true at the component level at all... just set <Provider react15CompatibilityMode={false}> .

Once React v16 becomes the popular choice, we can switch the default from true to false. Once v15 support is dropped, we can backout this change completely.

timdorr · 2016-11-09T21:31:53Z

src/components/Provider.js

@@ -20,7 +20,7 @@ function warnAboutReceivingStore() {

 export default class Provider extends Component {
  getChildContext() {
-    return { store: this.store }
+    return { store: this.store, react15CompatibilityMode: this.props.react15CompatibilityMode }


This shouldn't be on <Provider>. It's on a per-component basis, like pure. I wouldn't worry about context at all.

Provider is where you set the default, so you don't have to change it on every single connected component when you want the new behavior.

But you shouldn't be blanket applying a perf-reducing setting when it may not apply to most connected components. Just make it default at the connect() level and remove the context stuff.

I interpreted @gaearon's messages as we wanted to impose the least amount of breaking changes as possible, that's why I made it a global default setting which is "work like it used to, even if that means being not as fast as possible and maybe get stale props". It's certainly less code in react-redux to not have a global setting and require devs to go enable the compat setting on all their connected textboxes. I'm fine with either approach but I just need to be told which way we're going.

Correct, and for 5.0 it will be opt-out of the slower (but more compatible) behavior. For 5.1, which will wait on React 16, it will be opt-in for those who need to stick with React <=15.

timdorr · 2016-11-12T23:40:51Z

Have you run any perf tests on this? I assume it basically gets us back to 4.0 levels of speed, correct? That should be fine, since we're not regressing on speed at all. We can make a big warning label at the top of the release notes about it so no one misses the opt-out.

jimbolla · 2016-11-15T20:53:00Z

@timdorr Currently swamped with other things. Will perf test when I have a large block of time. Hopefully this weekend.

timdorr · 2016-11-21T03:17:10Z

Added a commit to remove the Provider API as requested. Again, we should be having people set this on a per-component basis, defaulting to true for 5.0 and defaulting to false for 5.1. This ensures a safe release that doesn't upset users, while still giving them an option to enable the more performant option if they deem it safe.

Also, I rebased this against the latest next, so the tests are now running against React 15. I'm sure the same behavior existed in 0.14, but this ensures we're dealing with the most popular version in use.

gnoff · 2016-11-21T05:48:40Z

@timdorr why remove the provider api? I want to use this compat mode for redux-form components but want everything else in the app to use the efficient sub ordering. Having to set this on each connected component seems an unnecessary chore. have it be defaulted to true in 5.0 but why make opting into performance more difficult than it needs to be?

timdorr · 2016-11-21T06:33:10Z

Just wrap connect and curry the options:

export function fastConnect(a,b,c,options) {
  return connect(a,b,c, { react15CompatibilityMode: false, ...options }) 
}

(On my phone, so I hope that came out right...)

gnoff · 2016-11-21T07:19:29Z

This works but it also means code modding / find replacing a large number of files with the need to do so again as soon as 5.1 comes out. I'm not adamant that this get added back in but the trade offs aren't cost free for users ( I realize they aren't cost free for the lib either since it requires supporting ). I could be misguided here but I think the cost is higher on lib consumers. Just lending my annecdote to @jimbolla intuition that a configurable global default might be desired.

jimbolla · 2016-11-21T14:45:04Z

@timdorr I'll concede taking it out of provider, but we should leave in the ability to pass the setting from props/context. Props is especially important if you want to control it through a library such as redux-form that hides the connect call.

timdorr · 2016-11-21T15:56:07Z

Sorry, I'm not trying to be dismissive. Obviously I want this to be an awesome release for most users. I haven't run into a situation like this before where we essentially need to define a global to configure a library. I do want to get this right and not just shove something somewhere, because I feel like we'll live to regret that decision 😱

What about a singleton module?

import { render } from 'react-dom'
import { setReact15CompatibilityMode } from 'react-redux'

setReact15CompatibilityMode(false)

render(<Provider store={store} children={appyStuffs} />, root)

It could also be generic: setDefaultOptions({ ... })

jimbolla · 2016-11-21T19:46:13Z

Would it make sense to just put the method on connect instead of being a top level export? Like this:

connect.setReact15CompatibilityMode(false);

I think it makes it more clear that it's a setting that affects connect like this.

markerikson · 2016-11-21T19:48:09Z

Frankly it all seems ugly any way you look at it, but I could get behind adding a method to connect like that.

timdorr · 2016-11-21T19:49:07Z

@jimbolla Yeah, I think that works better. Maybe call it connect.setDefaultReact15CompatibilityMode() so it's clear that it's global and changes the default.

…patibilityMode function.

jimbolla · 2016-11-21T22:16:02Z

Added connect.setDefaultReact15CompatibilityMode() to this PR

gnoff · 2016-11-21T22:17:02Z

@jimbolla @timdorr @markerikson thanks all

timdorr · 2016-11-28T21:33:26Z

Cool. Let's do this thing!

* Adds react15CompatibilityMode setting as temporary fix for reduxjs#525 * Remove Provider API for react15CompatibilityMode. * Fix tests against react15CompatibilityMode. * adds defaultReact15CompatibilityMode setting and setDefaultReact15CompatibilityMode function.

timdorr requested changes Nov 9, 2016

View reviewed changes

jimbolla modified the milestones: 5.1, 5.0 Nov 11, 2016

timdorr added the bug label Nov 12, 2016

timdorr changed the title ~~Adds react15CompatibilityMode setting as temporary fix for #525~~ Adds react15CompatibilityMode setting Nov 21, 2016

timdorr approved these changes Nov 21, 2016

View reviewed changes

jimbolla and others added 3 commits November 20, 2016 22:11

Adds react15CompatibilityMode setting as temporary fix for #525

7d7160b

Remove Provider API for react15CompatibilityMode.

ab12dcb

Fix tests against react15CompatibilityMode.

d522d63

adds defaultReact15CompatibilityMode setting and setDefaultReact15Com…

82ea838

…patibilityMode function.

jimbolla mentioned this pull request Nov 28, 2016

Error when using HMR #513

Closed

timdorr merged commit f41df99 into reduxjs:next Nov 28, 2016

timdorr mentioned this pull request Nov 28, 2016

Connected text input cursor position reset on input #525

Closed

jimbolla deleted the compatibilityMode branch November 29, 2016 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds react15CompatibilityMode setting #540

Adds react15CompatibilityMode setting #540

jimbolla commented Nov 9, 2016

timdorr Nov 9, 2016

jimbolla Nov 9, 2016

timdorr Nov 11, 2016

jimbolla Nov 11, 2016

timdorr Nov 12, 2016

timdorr commented Nov 12, 2016

jimbolla commented Nov 15, 2016

timdorr commented Nov 21, 2016

gnoff commented Nov 21, 2016

timdorr commented Nov 21, 2016 •

edited

Loading

gnoff commented Nov 21, 2016

jimbolla commented Nov 21, 2016

timdorr commented Nov 21, 2016 •

edited

Loading

jimbolla commented Nov 21, 2016

markerikson commented Nov 21, 2016

timdorr commented Nov 21, 2016

jimbolla commented Nov 21, 2016

gnoff commented Nov 21, 2016

timdorr commented Nov 28, 2016

Adds react15CompatibilityMode setting #540

Adds react15CompatibilityMode setting #540

Conversation

jimbolla commented Nov 9, 2016

timdorr Nov 9, 2016

Choose a reason for hiding this comment

jimbolla Nov 9, 2016

Choose a reason for hiding this comment

timdorr Nov 11, 2016

Choose a reason for hiding this comment

jimbolla Nov 11, 2016

Choose a reason for hiding this comment

timdorr Nov 12, 2016

Choose a reason for hiding this comment

timdorr commented Nov 12, 2016

jimbolla commented Nov 15, 2016

timdorr commented Nov 21, 2016

gnoff commented Nov 21, 2016

timdorr commented Nov 21, 2016 • edited Loading

gnoff commented Nov 21, 2016

jimbolla commented Nov 21, 2016

timdorr commented Nov 21, 2016 • edited Loading

jimbolla commented Nov 21, 2016

markerikson commented Nov 21, 2016

timdorr commented Nov 21, 2016

jimbolla commented Nov 21, 2016

gnoff commented Nov 21, 2016

timdorr commented Nov 28, 2016

timdorr commented Nov 21, 2016 •

edited

Loading

timdorr commented Nov 21, 2016 •

edited

Loading