[12:02 PM] jimbolla: Oh hey that's me. (The complete rewrite, not the GIF guy). I'd love to hear your thoughts on that work.
[1:23 PM] acemarke: @jimbolla my thoughts on the topic are... mixed, at the moment
[1:23 PM] acemarke: first, I'd definitely agree that the implementation of connect() has gotten significantly hairier and harder to follow as support for additional optimizations and use cases has been added
[1:24 PM] acemarke: which is why there's that PR/discussion of trying to separate the implementation of caching and such
[1:26 PM] acemarke: second, you seem to be a pretty productive dev, have put actual time/thought/effort into your reimplementation, and are taking a reasonable approach to offering it up for discussion
[1:29 PM] acemarke: third: as I said in that "gif-ful" pull 1813 discussion, I have commit access, but basically limit my actual repo activities to doc improvements and responding to issues. I do have opinions, but don't generally see myself as a final arbiter for code changes.
[1:30 PM] acemarke: that said, my opinions on code changes at this point tend to be pretty conservative. The Redux and React Redux APIs are pretty stable at this point, they're intentionally minimal, and there's been a corresponding explosion of ideas, options, and libraries in the ecosystem.
[1:31 PM] acemarke: so, I'm hesitant to put support behind changes unless they offer a significant benefit in performance or capability
[1:33 PM] acemarke: again, case in point, that "batched actions" PR. Sure, it was "just" allowing the action arg to be an array and using a for loop, but it's a niche use case, there's a "blessed" userland implementation, and it's one less thing that has to be built-in
[1:34 PM] acemarke: on the other hand, there's been tons of demand for a way to use combineReducers to manage per-key slices of state but still be able to access other slices as needed. So, when someone submitted yet another PR to add support for a third "all the state" argument to combineReducers, Dan decided it was probably time to let that proceed and see how it looks.
[1:35 PM] acemarke: and I agreed that seemed like a pretty reasonable idea.
[1:37 PM] acemarke: similarly, combineReducers is limited to working with plain JS objects, but a lot of people use Immutable.js for their state, or even other structures
[1:38 PM] acemarke: and there's been several reimplementations of combineReducers to work with Immutable objects
[1:39 PM] acemarke: so Dan proposed allowing configuration of how combineReducers iterates an object and reassembles it, allowing you to interop with different libs while still using the common shape checking logic and such. Again, seems like a pretty reasonable idea
[1:40 PM] acemarke: so all that said: I'm not against making changes to the implementation of connect(), and I'm definitely in favor of improvements to its maintainability
[1:40 PM] acemarke: but given that the existing code is there, has been widely tested, and works pretty well, I'm leery of a complete rewrite, even if it does pass all the existing tests
[1:41 PM] acemarke: at the moment, yours apparently doesn't handle a couple of the factory function use cases, and also introduces a new dependency on Reselect. Granted, Reselect is widely used with Redux, and even part of the same ReactJS org, but on general principle I would be hesitant to make that dependency a requirement.
[1:42 PM] acemarke: (erm. sorry, didn't realize I was gonna Wall O' Text that much)
[1:43 PM] acemarke: so, summary: at this point I would probably be against unless there's some specific demonstrated improvements in speed, maintainability, or use cases. And, given that I'm just "an opinion", I'd really like to hear from others who have actually worked on the code and know what they're doing with it (Dan in particular)
[1:46 PM] acemarke:
[1:51 PM] jimbolla: @acemarke Actually I got the factory use cases working. So I'm 100% API compatible with all tests passing ATM. I'd argue that splitting connect into connect and connectAdvanced would be more maintainable and probably allow users to solve some of their edge cases in the own code by using connectAdvanced instead of connect, leading to less requests for changes. That's what I was looking to do originally before I undertook this rewrite but the current API didn't have enough extension points to do so. I appreciate the wall of text. feedback = good. ??
[1:53 PM] jimbolla: I don't think adding a dependency on Reselect is a big deal. It's 87 lines of code. I think my rewrite is actually more than that many lines of code less than the current.
[1:53 PM] acemarke: well, full API compat / all tests passing is certainly a good start
[1:54 PM] acemarke: suppose the next question is, what are the net benefits? Any meaningful performance improvements? What additional use cases are supported, and how? How does this change in terms of the state update workflow?
[1:58 PM] jimbolla: performance of running the tests is almost identical. I ran the old and the new in a loop about 700 times and the average diff was 3ms which at that precision, could be impacted by background CPU tasks. But performance in a long running app could be better, given the extra caching related to selector memoization. We'd need some proper performance tests to determine that. I
[1:59 PM] acemarke: so on that note, here's some benchmark-y repos you might want to play around with using your implementation:
[1:59 PM] acemarke: https://github.com/mweststrate/redux-todomvc , https://github.com/jscriptcoder/stressing-redux , https://github.com/broadsw0rd/react-redux-perf
GitHub
mweststrate/redux-todomvc
redux-todomvc - Redux todoMVC, used to do some benchmarking
GitHub
jscriptcoder/stressing-redux
stressing-redux - Testing performance in redux architecture
GitHub
broadsw0rd/react-redux-perf
react-redux-perf - Dead simple performance test of the official React bindings for Redux
[2:02 PM] jimbolla: cool. I will check those out.
[2:02 PM] jimbolla: I'm not sure what you meant by "How does this change in terms of the state update workflow?"
[2:02 PM] acemarke: so right now, the basic steps are:
[2:03 PM] acemarke: 1) Subscription callback puts current state value into component state with this.setState(), queueing a React re-render. (That means that if multiple actions are dispatched quickly, React's batching will probably only perform one wrapper component re-render with the final value.)
[2:04 PM] acemarke: 2) mapState is called in the wrapper component re-render process and diffed vs the last result to determine if the store update changed anything this component cares about
[2:05 PM] acemarke: 3) If something did change, then the wrapper component actually does re-render the child. Otherwise, it not only doesn't re-render, it returns the exact same rendered React Element definition it returned last time
[2:05 PM] acemarke: I think one of those latter two benchmark repos was saying that the setState usage was adding up to be a high CPU percentage or something
[2:06 PM] acemarke: so, I'm curious what the high-level steps are for your approach, and how they differ
[2:06 PM] jimbolla: my process is pretty similar. i think it's the same "high-level"
[2:18 PM] jimbolla: one difference is with my solution. the reconciliation work is all done during shouldComponentUpdate, so render() doesn't actually need to fire if the final props haven't changed.
[2:20 PM] acemarke: would probably be good to document both the approach concepts and the "new" abilities in that discussion
[2:23 PM] totaldis: I should really read the connect source some time
[2:24 PM] totaldis: I'll probably let this potential PR work it's way through first though
[2:24 PM] acemarke: Dan wrote an uber-simplified version at https://gist.github.com/gaearon/1d19088790e70ac32ea636c025ba424e
Gist
connect.js explained
connect.js explained
[2:24 PM] totaldis: yea, I saw that(edited)
[2:24 PM] acemarke: shows the basic data flow, but leaves out all the caching concepts
[2:24 PM] totaldis: looking for the fully monty
[2:24 PM] jimbolla: I'm working on adding comments to the source now. If you look at latest, there's some in there: https://github.com/jimbolla/react-redux/blob/connect-rewrite/src/components/connectAdvanced.js
GitHub
jimbolla/react-redux
react-redux - Official React bindings for Redux