fix leak on server side, not store singleton instance to global object #100

kaesonho · 2016-08-06T01:18:54Z

we are storing rootI13nNode and reactI13nInstance on the global object, which causes memory leak, we are leaking all the i13n react component in memory on server side, because they eventually have a reference to the rootI13nNode

the solution here is just let setupI13n create that instance and pass it through context,
so that each I13n component can still get the instance.
also rootI13nNode can just store in the reactI13n instance.

After the change, we can see there's no leaking on i13nNodes now

yahoocla · 2016-08-06T01:18:55Z

CLA is valid!

coveralls · 2016-08-06T01:20:54Z

Coverage decreased (-0.1%) to 90.777% when pulling ad3be89 on fixleak into ce36e52 on master.

lingyan · 2016-08-06T02:39:10Z

src/libs/ReactI13n.js

@@ -26,6 +25,7 @@ if ('client' === ENVIRONMENT) {
 * @param {Object} options.rootModelData model data of root i13n node
 * @param {Object} options.i13nNodeClass the i13nNode class, you can inherit it with your own functionalities
 * @param {String} options.scrollableContainerId id of the scrollable element that your components
+ * @param {String} options.context context object


make it clear this is the component context, and link to fluxible doc for component context.

coveralls · 2016-08-06T05:10:28Z

Coverage decreased (-0.1%) to 90.777% when pulling 4cda8d9 on fixleak into ce36e52 on master.

coveralls · 2016-08-06T06:13:11Z

Coverage remained the same at 90.909% when pulling 015f419 on fixleak into ce36e52 on master.

coveralls · 2016-08-06T09:59:57Z

Coverage increased (+0.04%) to 90.953% when pulling 291d189 on fixleak into ce36e52 on master.

coveralls · 2016-08-08T16:37:50Z

Coverage increased (+0.1%) to 91.013% when pulling 65e5a54 on fixleak into ce36e52 on master.

kaesonho · 2016-08-08T16:38:43Z

@lingyan @redonkulus @longlho updated, please check again.

coveralls · 2016-08-08T16:45:30Z

Coverage increased (+0.1%) to 91.013% when pulling dea7f24 on fixleak into ce36e52 on master.

lingyan · 2016-08-08T16:47:58Z

src/mixins/I13nUtils.js

-                parentI13nNode: this._i13nNode
-            }
+                parentI13nNode: this._i13nNode,
+                reactI13nInstance: this._getReactI13n()


document how people can use this new context field

this is internally usage, we use this context to pass reactI13n instance(and the i13n root) across components, do we want users to know this?

kaesonho · 2016-08-09T00:21:40Z

@lingyan @redonkulus updated, I rollbacked the breaking change, added document for the new version change, will do a minor version bump.

coveralls · 2016-08-09T00:25:33Z

Coverage decreased (-0.3%) to 90.59% when pulling 2a6c84b on fixleak into ce36e52 on master.

coveralls · 2016-08-09T04:22:45Z

Coverage increased (+0.2%) to 90.59% when pulling 9203666 on fixleak into 858e80a on master.

coveralls · 2016-08-09T04:26:20Z

Coverage decreased (-0.7%) to 89.731% when pulling 9203666 on fixleak into 858e80a on master.

redonkulus · 2016-08-09T17:49:52Z

docs/api/setupI13n.md

@@ -27,6 +27,15 @@ var I13nDempApp = setupI13n(DemoApp, {
 }, [someReactI13nPlugin]);



looks like you are missing closing back ticks for the example. This messes up formatting below.

redonkulus · 2016-08-09T17:50:27Z

docs/api/setupI13n.md

+
+What we do with `setupI13n` is that we will create the `ReactI13n` instance, along with a root node of the I13nTree, passing them via component context to the children. 
+
+It's designed to work within React components, you should be able to just [utilFuctions](https://github.com/yahoo/react-i13n/blob/master/docs/guides/utilFunctions.md) and trigger i13n events. In case you want to do this out of React components, you can access `window._reactI13nInstance` directly.


should be able to just use utilFunctions

kaesonho · 2016-08-09T22:19:59Z

updated @redonkulus @lingyan please check

coveralls · 2016-08-09T22:21:52Z

Coverage decreased (-0.7%) to 89.731% when pulling 8754f9c on fixleak into 858e80a on master.

redonkulus · 2016-08-09T22:27:42Z

👍

coveralls · 2016-08-10T06:16:03Z

Coverage decreased (-1.2%) to 89.185% when pulling f5a6a90 on fixleak into 858e80a on master.

lingyan · 2016-08-10T08:08:16Z

lingyan reviewed Aug 6, 2016
View reviewed changes

kaesonho force-pushed the fixleak branch from ad3be89 to 4cda8d9 Compare August 6, 2016 05:09

kaesonho force-pushed the fixleak branch from 4cda8d9 to 015f419 Compare August 6, 2016 06:11

kaesonho changed the title ~~use context as global object to fix leak on server side~~ [WIP]use context as global object to fix leak on server side Aug 6, 2016

kaesonho force-pushed the fixleak branch from 015f419 to 177392f Compare August 6, 2016 09:46

kaesonho changed the title ~~[WIP]use context as global object to fix leak on server side~~ use context as global object to fix leak on server side Aug 6, 2016

kaesonho force-pushed the fixleak branch from 177392f to 291d189 Compare August 6, 2016 09:58

kaesonho changed the title ~~use context as global object to fix leak on server side~~ fix leak on server side, not store singleton instance to global object Aug 6, 2016

kaesonho force-pushed the fixleak branch from 291d189 to 65e5a54 Compare August 8, 2016 16:35

kaesonho force-pushed the fixleak branch from 65e5a54 to dea7f24 Compare August 8, 2016 16:41

lingyan reviewed Aug 8, 2016
View reviewed changes

kaesonho force-pushed the fixleak branch 2 times, most recently from 1d9d2d7 to 9203666 Compare August 9, 2016 04:21

redonkulus reviewed Aug 9, 2016
View reviewed changes

kaesonho force-pushed the fixleak branch from 8754f9c to f5a6a90 Compare August 10, 2016 06:10

not store instance to global, remove custom I13nNode support

f5a6a90

kaesonho merged commit 9e5392f into master Aug 10, 2016

kaesonho deleted the fixleak branch August 10, 2016 08:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix leak on server side, not store singleton instance to global object #100

fix leak on server side, not store singleton instance to global object #100

kaesonho commented Aug 6, 2016 •

edited

Loading

yahoocla commented Aug 6, 2016

coveralls commented Aug 6, 2016 •

edited

Loading

lingyan Aug 6, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading

coveralls commented Aug 8, 2016 •

edited

Loading

kaesonho commented Aug 8, 2016

coveralls commented Aug 8, 2016 •

edited

Loading

lingyan Aug 8, 2016 •

edited

Loading

kaesonho Aug 8, 2016

kaesonho commented Aug 9, 2016 •

edited

Loading

coveralls commented Aug 9, 2016 •

edited

Loading

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

redonkulus Aug 9, 2016

redonkulus Aug 9, 2016

kaesonho commented Aug 9, 2016

coveralls commented Aug 9, 2016 •

edited

Loading

redonkulus commented Aug 9, 2016

coveralls commented Aug 10, 2016 •

edited

Loading

lingyan commented Aug 10, 2016

		@@ -27,6 +27,15 @@ var I13nDempApp = setupI13n(DemoApp, {
		}, [someReactI13nPlugin]);


		What we do with `setupI13n` is that we will create the `ReactI13n` instance, along with a root node of the I13nTree, passing them via component context to the children.

		It's designed to work within React components, you should be able to just [utilFuctions](https://github.com/yahoo/react-i13n/blob/master/docs/guides/utilFunctions.md) and trigger i13n events. In case you want to do this out of React components, you can access `window._reactI13nInstance` directly.

fix leak on server side, not store singleton instance to global object #100

fix leak on server side, not store singleton instance to global object #100

Conversation

kaesonho commented Aug 6, 2016 • edited Loading

yahoocla commented Aug 6, 2016

coveralls commented Aug 6, 2016 • edited Loading

lingyan Aug 6, 2016 • edited Loading

Choose a reason for hiding this comment

coveralls commented Aug 6, 2016 • edited Loading

coveralls commented Aug 6, 2016 • edited Loading

coveralls commented Aug 6, 2016 • edited Loading

coveralls commented Aug 8, 2016 • edited Loading

kaesonho commented Aug 8, 2016

coveralls commented Aug 8, 2016 • edited Loading

lingyan Aug 8, 2016 • edited Loading

Choose a reason for hiding this comment

kaesonho Aug 8, 2016

Choose a reason for hiding this comment

kaesonho commented Aug 9, 2016 • edited Loading

coveralls commented Aug 9, 2016 • edited Loading

coveralls commented Aug 9, 2016

coveralls commented Aug 9, 2016

redonkulus Aug 9, 2016

Choose a reason for hiding this comment

redonkulus Aug 9, 2016

Choose a reason for hiding this comment

kaesonho commented Aug 9, 2016

coveralls commented Aug 9, 2016 • edited Loading

redonkulus commented Aug 9, 2016

coveralls commented Aug 10, 2016 • edited Loading

lingyan commented Aug 10, 2016

kaesonho commented Aug 6, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading

lingyan Aug 6, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading

coveralls commented Aug 6, 2016 •

edited

Loading

coveralls commented Aug 8, 2016 •

edited

Loading

coveralls commented Aug 8, 2016 •

edited

Loading

lingyan Aug 8, 2016 •

edited

Loading

kaesonho commented Aug 9, 2016 •

edited

Loading

coveralls commented Aug 9, 2016 •

edited

Loading

coveralls commented Aug 9, 2016 •

edited

Loading

coveralls commented Aug 10, 2016 •

edited

Loading