React-redux: v5 release chores

[jimbolla]

Starting a list of things related getting v5 out the door.

• [x] Unhide new features - currently they are hidden so that nobody starts using them, just in case we roll this back. I'll make a PR (#474) that will unhide them that can be merged in once we're confident that these changes will go live.
• [x] Create release notes - needs to describe: still API compatible, synopsis of internal architectural changes, perf changes, bugs fixed, new features added
• [x] Review/revise release notes @timdorr toughts?
• [x] Write documentation for new features. Need to adequately describe connectAdvanced, as well as new options passable to connect. PR #480
• [x] Review/revise new docs. @timdorr @markerikson toughts?
• [x] Fix controlled input issues #525
• [x] Release another beta with the new features exposed. Add beta to the github Releases page.

Nice to Haves:

• Refactor tests? - now that connect is split into to conceptual pieces: connectAdvanced for store subscription and component lifecycle stuff vs connect's selector functions, many of the tests could be refactored to deal with just one or the other. This would be nice because would split the 2000+ line spec file in two, making it a little easier to work with. Also add new tests for some of the new connect options. (I personally need to learn how to run the new test coverage tools so I can see what's not covered.)

[timdorr]

I already updated the code linting (#461), so that's taken care of. I intentionally didn't use airbnb because I didn't want to change styles just for the sake of changing styles.

Flow and Typescript typings both have PRs open (#389 and #433) respectively. They have atrophied, but I'm not a user of either typing system, so I can't really champion either effort. That has to come from the community.

[timdorr]

If you want to update the API.md with some initial changes or documentation placeholders, I can collaborate with you on that to put it over the finish line. That's really the key thing needed to get to 5.0.0 final.

[jimbolla]

react-redux v5 release notes (WIP)

TL;DR

• Backwards compatible API
• Major internal changes
• Significant performance improvements in common usage patterns
• Bugs fixed
• Additional features added to connect()
• New connectAdvanced() API

API Compatibility

Version 5.0 maintains API compatibility with v4.x but due to major internal changes, we felt a major version bump was warranted. Store state change notifications sent to components are now guaranteed to occur top-down, so if your code may behave differently if it relied on notifications happening out of order.

Internal Changes

Internally, the code for connect has been rewritten from the ground-up to be more modular, with the intention of greater maintainability and extensibility. This has also led to some new features and performance improvements.

Performance improvements

Significant performance gains were achieved by avoiding extra calls to setState() and render() by ensuring the order of change notifications to occur top-down, matching React's natural flow. Performance tests and benchmarks are discussed in #416.

Bugfixes

Some bugs/issues are resolved, related to performance loss and also impure components not re-rendering.

New features added to connect()

The behavior of connect() is now more customizable, by passing additional properties in the options arg. These will be described in the API docs.

New top level API: connectAdvanced()

The new implementation of connect() splits its behavior such that it is now a wrapper around a more-generalized connectAdvanced(). This connectAdvanced() method can be called directly if you have extreme performance needs or want to craft an API different than that of connect(). This will be described in the API docs.

[jimbolla]

There's a start for the release notes. I'll start on the api.md and submit that as another PR once I have something substantial.

[markerikson]

It'd be nice to get the API docs more "official"-looking. At the moment, they're just buried in the repo, which is not terribly obvious. Can we Gitbook-ify them, publish them, and also add a pointer from the Redux docs to the React-Redux docs?

[jimbolla]

I'm unfamiliar with Gitbook... what does that conversion entail?

[timdorr]

I think we can just reorganize the Readme a bit. A gitbook isn't really needed for only a couple pages of docs. The main Redux docs cover usage info for this repo anyways. We're better off leaving this one to just the basics.

[markerikson]

Okay. Can we at least stick some specific shorter "manual" <a> tags in there, so that it's possible to link directly to one of the API descriptions without having a hash anchor that's longer than the API description? :)

[jimbolla]

I started updating docs here, but ran out of time tonight.

[jimbolla]

There's an ok start to the docs update in PR #480. Would love some CC.

[jimbolla]

@markerikson Can you give me an example of what to add in there for manual tags and I'll update the docs this weekend?

@timdorr Thoughts on adding the beta release to the github releases tab?

[markerikson]

@jimbolla : You can stick HTML straight into Markdown, so what I did was insert <a> tags right before each appropriate section header. I gave each anchor a custom relevant shorter name, as opposed to the long auto-generated names from the slugified header text:

<a id="general-only-react"></a>
### Can Redux only be used with React?

[jimbolla]

@markerikson I threw some a tags in PR #480

[timdorr]

Sorry I've been captain lazypants on this stuff. I hope to get to some of it in the next few days (namely another beta release).

[jimbolla]

@timdorr It's all good. I appreciate that everyone has their own things going on. I just want to make sure this doesn't get lost.

[timdorr]

Finally pushed out beta 2: https://github.com/reactjs/react-redux/releases/tag/v5.0.0-beta.2

Sorry, I had an eventful week (lots of personal issues; check my Twitter feed), so I haven't had time for this lately :|

[timdorr]

• Move the test refactoring out of the checklist. It's not needed to go final.
• Added #507 as a task because that really needs to land.
• Also added a task for #525, so we don't run into trouble with controlled inputs.

Almost there...

[timdorr]

All set!

I'm closing on a house on Wednesday, so can we do this at the end of the week? I can push an RC out tonight just to check any last-minute issues and prep the release notes and such.

[jimbolla]

@timdorr Let me know if I can help.

[timdorr]

Sorry, I got tied up with some last-minute house things last night. Will try to get to that release soon.

[jimbolla]

No sweat. Enjoy your new house and new Tesla, moneybags. 😆

[timdorr]

OK, obviously didn't get to this over the weekend as promised. I'll try to push something out today. Just have to prepare the release notes and we'll be good.

[ghigt]

I have a regression between the beta3 and the rc1.
Children are called before the parent after a change.
I had the same problem with the v4 but it was solved using beta3 of v5.

[jimbolla]

@ghigt we added a setting in rc1 to make it opt-in, but then removed it in latest. there isn't a release w/ the latest. see the release notes for rc1 for details.

[jimbolla]

@timdorr I think everything's good to go. Perf tests look good. The only known issue is broken HMR (#513) which I can spend some time fixing while on christmas vacation. I added a note about that to the release notes draft, which look good.

[timdorr]

Moving is haaard! 😭

[gnoff]

@timdorr Can we publish an rc2 with the expected 5.0 change?

[timdorr]

I'm just going to go hardcore 5.0 whenever I have a chance. I'm moving the big stuff on Sunday (I'm doing what I can do in my car over the past week and through the weekend), so I should definitely be free by then. I keep trying to come up with time for it, but I also enjoy sleep 😜

[jimbolla]

@gnoff If you're eager to test it, I published the latest changes as @jimbolla/react-redux. You can npm install that and then if you're using webpack, you can alias that on top of react-redux proper:
```js
{
resolve: {
alias: {
'react-redux': '@jimbolla/react-redux',
}
}
}

[ghigt]

@jimbolla sorry, I didn't understand well the release note. As I don't have any issue with the beta3, I'll keep it until the flag is removed. Thanks for your reply.

[gaearon]

Refactor tests? - now that connect is split into to conceptual pieces: connectAdvanced for store subscription and component lifecycle stuff vs connect's selector functions, many of the tests could be refactored to deal with just one or the other. This would be nice because would split the 2000+ line spec file in two, making it a little easier to work with.

I’d like to offer some words of caution about this one. We recently went through some pains rewriting React tests in terms of public API. We are rewriting its internals, and so unit tests for old internals turned out to be useless and sometimes complicated to port.

I’m still not sure if the top-down subscription approach will pan out long term. I haven’t thought about it really. I don’t want to be the grumpy person here and I appreciate all the effort, but relying on lifecycle order feels fragile, and reliant on React implementation details. I’m not sure this exact approach will still work with Fiber in the future. React has the batching feature for this, and it will eventually batch everything by default. Maybe we can just flip the implementation then.

In any case, I would prefer that we keep tests based around public API whenever possible. It’s fine to add more granular unit tests but any important behavior should have an integration test. Otherwise it will be tricky to change the underlying implementation later if we need to.

[markerikson]

I admittedly only really looked through the internals the one time during the work on #416, and haven't dug through them since. But: I thought the point of the top-down subscriptions was to _avoid_ being dependent on lifecycle ordering.

[markerikson]

Just saw your Reddit comment at https://www.reddit.com/r/reactjs/comments/5hf4d4/an_artificial_example_where_mobx_really_shines/db09sf2/ . Quoting for posterity:

To be honest I'm still feeling wary about subscribing in order. This feels like a hack for something React can already do in the batched mode. But I'm not actively involved now so it's hard for me to say.

@jimbolla is way more familiar with the guts of this, and you're obviously _way_ more familiar with the guts of React, but as I understand it there's two main intended benefits:

• Ensure that we don't have children subscribing before parents, as can happen in 4.x. The current behavior results in nasty bugs if you're using the "connected parent passing IDs to connected children" pattern, as a child whose item just got deleted may try to retrieve no-longer-existing data before its parent has a chance to re-render without that child. Top-down subscriptions solve that by ensuring that the parent re-renders first, causing the child to unmount.
• By handling things top-down, it minimizes wasted effort, since a connected child won't call setState before its parent has.

[gaearon]

The current behavior results in nasty bugs if you're using the "connected parent passing IDs to connected children" pattern, as a child whose item just got deleted may try to retrieve no-longer-existing data before its parent has a chance to re-render without that child.

Right, but wouldn’t this problem cease to exist were all setStates batched by React and executed in the correct order in the future? I’m just trying to figure out the longer term plan.

since a connected child won't call setState before its parent has.

Longer term this shouldn't matter. (That's what batching solves.)

But: I thought the point of the top-down subscriptions was to avoid being dependent on lifecycle ordering.

It is dependent on setState callbacks which seems unfortunate to me because it forces React to keep track of them. This will negate some benefits of Fiber if I understand it correctly. I’m totally fine with doing it if it’s necessary now but I just want us to keep a larger picture in mind and have a way out of this eventually.

[markerikson]

Right, but wouldn’t this problem cease to exist were all setStates batched by React and executed in the correct order in the future? I’m just trying to figure out the longer term plan.

Ah... as I understand it, no - it's the _subscription itself_ that's the issue. In other words, something like:

const mapState = (state, ownProps) => {
    const itemForThisComponent = state.items[ownProps.itemID];
    const {someValue} = itemForThisComponent;
    return {someValue};
}

If you deleted the item, then when mapState tries to read out that entry and extract further data from it, itemForThisComponent is going to be undefined, and the someValue read will blow up. Sure, you can (and probably should) put in defaults and failsafes, but not everyone will. I ran into this in my own app a few times, and it took me a while to catch on to the pattern.

[jimbolla]

It is dependent on setState callbacks which seems unfortunate to me because it forces React to keep track of them.

This isn't true anymore. I removed the setState callback and moved it to componentDidUpdate.

I added the top-down subscriptions to fix the stale props issue, as discussed in #292, as well as the your comments above. This ensures that mapStateToProps only gets called with fresh props.

The major perf issue in 4.5 was related to the way that the component's store listener would call setState for every store change, if the component's mapStateToProps depended on ownProps. #398 was one such report of this. The perf loss occurred even if the component ultimately didn't rerender. It seems just calling setState itself is costly enough.

My solution was to make sure the listener only has to call setState if it actually has new final props by calling mapStateToProps etc during the listener & before calling setState. So that work is now being done outside the React lifecycle. This is also what will enable us to unimplement shouldComponentUpdate, as discussed in #507.

In order to take advantage of React's batched updates, we'd have to bring the props calculate back into the React lifecycle. I haven't completely thought it through, but I believe that would involve:

• Going back to calling setState for every store state change, but wrapping it in a call to batchedUpdates.
• Computing new final props during componentWillUpdate
• Preserving shouldComponentUpdate

but relying on lifecycle order feels fragile, and reliant on React implementation details.

In my opinion, this way would be more reliant on React's implementation details.

[markerikson]

@jimbolla , side question: per your comments in https://github.com/dtinth/pixelpaint/pull/1#issuecomment-266211907 , any other tweaks you can think of that would help improve perf in that kind of scenario? I know, I know, all programming is tradeoffs and nothing is perfect, just would be nice to say that v5 is better in every way :)

[timdorr]

BTW, the hold-up on this release is me. I just did my move today and don't yet have reliable internet access (coming to you from my phone's hotspot temporarily). Once they can plug in the right wire outside my house, I can get this taken care of and welcome us to The Future™!

[timdorr]

Done 💥

[timdorr]

Obligatory tweet! https://twitter.com/timdorr/status/808898559097573376