search

paypal / react-engine

a composite render engine for universal (isomorphic) express apps to render both plain react views and react-router views
Apache License 2.0
1.45k stars 130 forks source link

Change Client From LoDash Merge to LoDash Assign #169

Closed JustinMGaiam closed 8 years ago

JustinMGaiam commented 8 years ago
JustinMGaiam commented 8 years ago

This greatly increase the router change speed as well. When using Redux and ImmutableJS data can get very expensive in terms of doing the deep copy's which is why ImmutableJS provides its own merge method. This replaces lodash's merge with assign so a shallow copy is performed.

It seems to me that a merge is unnecessary, if I understand what the client is doing correctly, when creating the router component. Unless you want props with the same name on both the router component and child component to combine together into a single prop as it does with merge, assign will just favor the router prop of the same name over the child component prop.

If this needs to be a more dynamic strategy I am curious what this is actually designed to do, but for now this works in our app and greatly improves performance which currently is unusable in both FireFox and Internet Explorer. We did not run into issue until our store data started to grow over time.

samsel commented 8 years ago

@JustinMGaiam thanks for fixing this. We just care about the root level copy, so assign should be perfectly fine. I've publish 4.0.1

samsel commented 8 years ago

@JustinMGaiam assign breaks react-engine if you have a property named the same as a property name in reactRouterProps. I have a PR sitting here: mind taking a look? - https://github.com/paypal/react-engine/pull/173