Changing Lodash Merge to Assign in Server

[JustinMGaiam]

This is the same fix from #169 which started with #166 but applied to the server side code. When I apply this update our render times server side drop by at least half.

[SOSANA]

@samsel LGTM please 👍

[JustinMGaiam]

@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]

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 #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]

@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]

@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]

@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.

[gabrielcsapo]

what is the advantage in selecting modules inside loads? instead of just keeping var _ = require('lodash');?

[zetlen]

@gabrielcsapo On the server, it can be a minor savings of space and time, depending on the complexity of the dependency graph. If you're using node-inspector to debug, it's good to load less unused code, since the inspector can bog down quickly. It's on the client where it's crucial, to save on bundle size.

[remarkablemark]

@zetlen I agree that you save some bytes and speed things up by a faction of a millisecond, but don't you feel it's a premature optimization?

Minimizing bundle size on the client is an excellent argument, but is this file even getting bundled at all? In the example, it specifically requires ./lib/client.

[zetlen]

@remarkablemark Yeah, as an optimization it's absolutely premature. As a style and consistency issue, it's kinda minor. I wouldn't hold up a merge for something like this.