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

Changing Lodash Merge to Assign in Server #172

Closed JustinMGaiam closed 7 years ago

JustinMGaiam commented 8 years ago

This is the same fix from https://github.com/paypal/react-engine/pull/169 which started with https://github.com/paypal/react-engine/issues/166 but applied to the server side code. When I apply this update our render times server side drop by at least half.

SOSANA commented 8 years ago

@samsel LGTM please :+1:

JustinMGaiam commented 8 years ago

@samsel if there are no issues with replacing merge on the server side as well this PR will cover the same change as on the client. This change cuts our server side render time in half or better.

zetlen commented 8 years ago

This is less important on the server than the client, but I still think it's always better to bring in lodash dependencies separately using the lodash-recommended require path, rather than repeatedly bringing in the whole library. I just submitted https://github.com/paypal/react-engine/pull/176 to fix this in the client library.

Again, it's less urgent than the client-side concern, but the lodash deps would run faster and be less memory-intensive on the server side if they were brought in like:

var isString = require('lodash/isstring');
var assign = require('lodash/assign');
var omit = require('lodash/omit');
samsel commented 8 years ago

@JustinMGaiam do we still need to change merge to assign in other places? I am afraid we'll be breaking a lot of things unknowingly

JustinMGaiam commented 8 years ago

@samsel we have been running this code on our Fork since I submitted this PR without an side effects and increased performance, plus it passes current tests. I will leave it up to you if you think this needs more testing or if the other PR at https://github.com/paypal/react-engine/pull/173/files that you already merged is enough to solve the issue. The merge here https://github.com/paypal/react-engine/pull/172/files#diff-c945a46d13b34fcaff544d966cffcabaR148 seems necessary since the user is providing all the data from the route and the only 2 things that could possible merge are data.view and data.markupId which I guess you might not want to overwrite anyway. The one thing I am unsure of is if the merge will essentially act like assign since the likely hood of having a key collision is low. If that is true all the performance is gained from the PR 173.

JustinMGaiam commented 8 years ago

@samsel also based on the comment from @zetlen looks like the only thing left that is in question is the merge on the data object. Everything else is in play from this PR.