RFC: Update middleware #182
Conversation
As mapPropsStream shows, many (most?) higher-order components can be described in terms of a props stream transformation. If multiple such higher-order components are composed together, we reduce the number of components by composing the transformations together instead, which should be more lightweight than sending props through React's update system at each level. In other words, the following two forms are equivalent: ```js // Wraps the base component in three container components compose( mapPropsStream(transformA) mapPropsStream(transformB) mapPropsStream(transformC) )(BaseComponent) // Same as this, which wraps the base component in a single // container component mapPropsStream( compose(transformA, transformB, transformC) )(BaseComponent) ``` But we don't want to require that Recompose users include an observable pollyfill. Instead, I'm proposing a lightweight abstraction called "update middleware" which will serve the same purpose. So far, I've implemented withState and withHandlers using this pattern. If we go in this direction, we will modify the compose function to detect HOCs defined using middleware and compose them using the second strategy from the example above.
| mapProps(props => ({ | ||
| ...omit(props, [propName]), | ||
| ...props[propName] | ||
| })) |
There was a problem hiding this comment.
In case it's not clear why this is included in this PR (other than being the right way to define flattenProp, anyway), the idea is that there are some helpers that we don't want to define using middleware, because middleware necessitates using a class component, even when a functional component would suffice. What we really want is to use a functional component unless the HOC is composed with another HOC which must use a class. Put another way, a single functional component is better than a class component, but a single class component is better than a class component and a functional component.
To achieve this, we will special case mapProps to use a functional component by default, but use middleware when being composed with other middleware-using HOCs.
|
Also, to be clear, this abstraction would be private: it would not be exposed to the public, except to advertise as a performance optimization. |
| if (shouldMap(this.props, nextProps)) { | ||
| this.computedProps = propsMapper(nextProps) | ||
| } | ||
| const update = (props, cb) => { |
There was a problem hiding this comment.
No need to create a update function here.
We can move next.update(...) to line 31.
|
I like this new middleware feature. I believe the new |
|
Yeah, exactly. The whole point is to solve #97. |
|
This is working out better than I thought. Turns out we don't even need a smart compose: recompose/src/packages/recompose/utils/createHocFromMiddleware.js Lines 88 to 93 in 132fe2f |
| destroy() { | ||
| this.xforms.forEach(xform => { | ||
| if (typeof xform.destroy === 'function') { | ||
| xform.destroy() |
There was a problem hiding this comment.
So the only need the middleware returns object is .destroy?
|
Just investigated a little, and looks like rxjs 5 does not give a lot overhead on dist size, And yes all this functions is written over mapPropsStream(flow(
mapProps(...),
withState(...)
))(BaseComponent) |
|
But what about the cost of importing a single module? I want to keep that as small as reasonably possible so that library authors can rely on individual helpers guilt free. |
|
Just an update: this PR is on hold pending further investigation. It's not clear to me there's any performance benefit to squashing components this way. If/when it can be proven that there is a significant performance boost, we will reevaluate. |
|
We should open an issue for adding performance testing |
|
There's still a benefit in terms of usability so even if there was no performance benefit I think it would still be worthwhile.
|
|
This is the approach used in recompact. I did some benchmarks, you can find it in the repo. Some conclusions from benchmarks:
However there is a true benefit in devtools. |
|
@neoziro Thanks for mentioning your project. It would be nice if you have a way to view the benchmark results without having to install and run them from your repo. Also include the summary like you have provided here. |
|
Updated Benchmarks with interesting results. |
|
The approach of reassemble is different, you are not exposing any hoc and it's not a drop-in replacement for recompose even if the API seems the same. Also you replace pure(withProps({ foo: 'bar' })(Component))
// or
compose(
pure,
withProps({ foo: 'bar' }),
)(Component)You lose the composition aspect of hoc. It reminds me of mixout. Also I updated recompact benchmarks and published the results (removing enzyme was the solution to stabilize them). My results are very different from yours: |
|
@neoziro As I mentioned reassemble is not using HOCs. I changed the project description to be more clear. I'm still puzzled by the differences in our two benchmarks. I can't spot the cause, anyone? I like the idea of recompact, in fact I considered a similar implementation. Atm I'm using reassemble in some of my projects and I believe initial render has almost cut in half . Though I need to do more performance testing. I'll keep an eye on recompact. |
|
@neoziro Your benchmark is incorrect. You need to cleanup on each run, otherwise the React reconciler kicks in and prevent any rerendering. |
|
Another difference between recompact and reassemble is that recompact cannot squash context related HOCs and the Lifecycle HOC. I often use lifecycle hooks for deferred rendering and to deal with animations (e.g. in my other open source project: react-css-transition). |
|
@cvle you are right, I will update benchmarks soon. |
|
@acdlite Will this be PR be merged in soon? |
|
@fried-chicken there are no reason for this now. The only issue is a React dev tools tree length. |
|
@istarkov I love recompose and use it frequently, but it can make debugging devtools kind of incomprehensible. This is especially true for other people on a team who didn't write the recomposed components and have to crawl through the component tree to try to figure out which file contains one particular occurrence of |
|
I dont use devtools so this is not a problem for me. And I love recompose because of simplicity of its code, IMO its so simple that does not need a docs. This change will make code less readable. So for me there are no advantages to apply this. |
Sorry, but from a maintainer of a project with 7.000 stars on github, and ~ 780.000 downloads each month from npm, this response feels kind of wrong. Don't get me wrong. The only reason for me to not switch over to |
|
How amount of stars or downloads can affect decisions? It's impossible to make project good for all so a metric "good for me" is very very good. If we'll start to accept every PR and every suggestion here it will be unusable project. |
|
The issue of devtools has been brought up, but not the one of serialization, particularly for snapshot tests with enzyme and Jest. When the component tree is so polluted, snapshot tests are going to change often, and the change diffs are not going to be readable. This makes snapshot testing too volatile to use in many cases. I know that this seems like a pretty specific use case, but 500,000+ people have downloaded enzyme-to-json in the past month. Are there no workarounds, or am I wrong? |
|
@rbong have you checked your words? Have you seen the snapshot generated by enzyme 2 json? |
|
@istarkov I mean for dev tools or other issues. You should lock this conversation. |
|
I think it's a matter of creating a special 3rd party Babel plugin to convert |
|
I think such a plugin would be awesome. But I'm not sure if it would work. This because In this snippet, export default compose(
connect(),
withHanders({
onClick: ({ dispatch }) => () => {
dispatch('foo');
},
}),
) |
|
@istarkov my apologies, I did not know enzyme-to-json contained a function to only render DOM nodes. |
|
@rbong not a problem :-) no need in apologies |
As
mapPropsStreamshows, many (most?) higher-order components can be described in terms of a props stream transformation. If multiple such higher-order components are composed together, we reduce the number of components by composing the transformations together instead, which should be more lightweight than sending props through React's update system at each level. In other words, the following two forms are equivalent:But we don't want to require that Recompose users include an observable pollyfill. Instead, I'm proposing a lightweight abstraction called "update middleware" which will serve the same purpose.
So far, I've implemented
withStateandwithHandlersusing this pattern.If we go in this direction, we will modify the compose function to detect HOCs defined using middleware and compose them using the second strategy from the example above.