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

Browser's memory keeps increasing when using React-engine & React-router #166

Open nguyenhhien opened 8 years ago

nguyenhhien commented 8 years ago

I'm writing Isormophic Web Application by using React-engine & React-router. Before upgrading, my application is running well with the following configuration. "react": "^0.14.8", "react-dom": "^0.14.6", "react-engine": "^3.4.1", "react-router": "^1.0.3" Recently, I've upgraded my system to use official React with the following configuration, and I observed my app become slower and slower when running on browser after every click I made on ReactRouter.Link. I've opened Google Chrome Debugger for investigation, then I see usage memory keep increasing after every click. "react": "^15.2.1", "react-dom": "^15.2.1", "react-engine": "^4.0.0", "react-router": "^2.5.2" Have anyone faced the similar problem? Please help me. Thank you :)

ghost commented 8 years ago

Having the same issue here. The render() method on the outermost layout keeps getting called. The example has the html tag in the outermost react-router layout route. I've never seen it done like that before; won't that cause the entire DOM to get re-rendered on every route switch and leave all the bindings orphaned?

I added more markup to the detail view of the example app and a Router.Link back to the list view. After 10 or so route changes (each of which call render() on the outer layout) the app starts slowing down and the memory footprint of the app in the browser continues to grow. This pattern (rendering the entire DOM from react-router) seems unworkable for anything but really tiny apps with minimal data binding or route nesting.

JustinMGaiam commented 8 years ago

We are encountering an issue with FireFox and IE where they become unusable quickly as our Immutable data in Redux grows after a few clicks which appears to be from out of control memory growth triggering rapid garbage collection. This does not happen in Chrome or Safari, so far I have tracked this down to lib/client.js line 97 https://github.com/paypal/react-engine/blob/master/lib/client.js#L97 does this look like the issue to anyone else?

If you change from lodash merge to lodah assign the issue seems to go away does this work for anyone else? Is there a reason .merge was used over .assign? Since our data is all Immutable.js the data is much bigger then standard object or array's so I think merge is going too deep when assign I believe is much more shallow. Let me know if the move to _.assign makes sense and I can do a super quick PR.

Also note that the route change speed is night and day with the switch to _.assign.

JustinMGaiam commented 8 years ago

If anyone wants to merge the PR it is here https://github.com/paypal/react-engine/pull/169 my only thought about this not working would be if the router props were some how the same name as the component props the router props would win instead of being merge together. Not sure anyone would expect two component with props of the same name to combine into a new "super" prop, but they would lose that with this change.

SOSANA commented 8 years ago

@JustinMGaiam you have an example of react-engine redux implentation I could checkout?

samsel commented 8 years ago

@nguyenhhien @keithchilders can you try 4.0.1 with @JustinMGaiam's fix?

JustinMGaiam commented 8 years ago

@SOSANA kraken-react-engine-router-redux-webpack is not my project, but it looks similar to how we came to our implementation since React Engine kind of forces you into attaching Redux to your high level app components since it cannot be attached to the Router props. To make this project closer to how we are using it you would need to hook up all of the Redux store using ImmutableJS and not plain Javascript object and arrays. When you are using ImmutableJS it does not take too much data to start seeing issues with merge if you are in IE or FireFox. Safari, Chrome, and Even Opera seem to do much better with processing very large arrays of data.

https://github.com/chunkiat82/kraken-react-engine-router-redux-webpack

SOSANA commented 8 years ago

@JustinMGaiam Thank you for replying back, much appreciated. I am using express but waiting for a fix for pr for #170 to fix an issue using provider with redux. That repo had me curious using kraken for future projects. Cheers!

JustinMGaiam commented 8 years ago

@SOSANA check out https://github.com/paypal/react-engine/pull/172 this is the same fix for the server as I applied to the client. Works equally as well and I missed it the first time around.

SOSANA commented 8 years ago

@JustinMGaiam Look forward to #172 merge :+1: in few days need to wire up react-engine with redux provider which I believe #171 merge now supports as well