Repository: DevExpress/devextreme-react (js)

Issue:
Event callbacks are reassigned to an inner widget on each render
<!--
  Note: if the issue is a general React question, please check React Docs first: https://reactjs.org/docs
  If your question is about  DevExtreme widgets' APIs, use the DevExpress Support Center: https://www.devexpress.com/Support/Center
-->

### Issue Type
bug report

### Description
<!-- Specify what exactly the issue is related to (e.g. a component name) -->
Event callbacks are reassigned to an inner widget on each render. It causes performance issues and "Maximum callstack" errors

#### Current Behavior
Event callbacks are reassigned to an inner widget on each render
#### Expected Behavior
Event callbacks are reassigned to an inner widget only then callbacks changes
#### Demo
<!--
  If you provide a minimal demo of the problem\proposal, it helps us to solve it much faster.
  Paste the link to your online code editor (like https://stackblitz.com) below:
-->
https://codesandbox.io/s/j4m8y7km05

### Environment Details
<!--
Specify DevExtreme version, DevExtreme React Components version, React version and other environment details or notes you consider important.
-->
- devextreme version: latest
- devextreme-react version: latest
- react version: latest


Pull request:
Fix event callbacks reassigning
Fix #221 
@IlyaKhD in fact, this is your solution, just refactored a bit and with real examples.

Patch:
diff --git a/example/data-grid-example.tsx b/example/data-grid-example.tsx
index 9568c53..3d717bb 100644
--- a/example/data-grid-example.tsx
+++ b/example/data-grid-example.tsx
@@ -1,7 +1,8 @@
 import * as React from "react";
 import Example from "./example-block";
 
-import CheckBox from "../src/ui/check-box";
+import { Template } from "../src/core/template";
+
 import DataGrid, {
     Column,
     FilterRow,
@@ -33,6 +34,10 @@ const RegionComponent = (props: any) => {
     return <b>{props.displayValue}</b>;
 };
 
+const ToolbarItemRender = () => {
+    return <b>Collapse or Expand Groups:</b>;
+};
+
 export default class extends React.Component<any, { expandAll: boolean, pageSize: number }> {
 
     constructor(props: any) {
@@ -42,7 +47,7 @@ export default class extends React.Component<any, { expandAll: boolean, pageSize
             pageSize: 5
         };
 
-        this.handleExpandAllChange = this.handleExpandAllChange.bind(this);
+        this.handleToolbarPreparing = this.handleToolbarPreparing.bind(this);
         this.handlePageIndexChange = this.handlePageIndexChange.bind(this);
     }
 
@@ -50,11 +55,6 @@ export default class extends React.Component<any, { expandAll: boolean, pageSize
         return (
             <Example title="DxDataGrid" state={this.state}>
                 <br />
-                <CheckBox
-                    text="Expand All Groups"
-                    value={this.state.expandAll}
-                    onValueChanged={this.handleExpandAllChange}
-                />
                 <br />
                 <br />
                 Page size:
@@ -69,6 +69,7 @@ export default class extends React.Component<any, { expandAll: boolean, pageSize
                 <DataGrid
                     dataSource={sales}
                     allowColumnReordering={true}
+                    onToolbarPreparing={this.handleToolbarPreparing}
                 >
                     <GroupPanel visible={true} />
                     <Grouping autoExpandAll={this.state.expandAll} />
@@ -92,16 +93,35 @@ export default class extends React.Component<any, { expandAll: boolean, pageSize
                         pageSize={this.state.pageSize}
                     />
                     <MasterDetail enabled={true} component={DetailComponent} />
+
+                    <Template name={"toolbarLabel"} render={ToolbarItemRender} />
                 </DataGrid>
             </Example>
         );
     }
 
-    private handleExpandAllChange(e: any) {
-        this.setState({
-            expandAll: e.value
+    private handleToolbarPreparing(args: any) {
+        args.toolbarOptions.items.unshift({
+            location: "after",
+            template: "toolbarLabel"
+          },
+          {
+            location: "after",
+            widget: "dxButton",
+            options: {
+                icon: "chevronup",
+                onClick: (e: any) => {
+                  this.setState((state) => {
+                    e.component.option("icon", state.expandAll ? "chevrondown" : "chevronup");
+                    return {
+                        expandAll: !state.expandAll
+                    };
+                  });
+                }
+            }
         });
-    }
+      }
+
     private handlePageIndexChange(e: any) {
         this.setState({
             pageSize: e.value
diff --git a/src/core/component.ts b/src/core/component.ts
index 3490b1a..21440c6 100644
--- a/src/core/component.ts
+++ b/src/core/component.ts
@@ -130,12 +130,14 @@ abstract class ComponentBase<P extends IHtmlOptions> extends React.PureComponent
       ...preparedProps.integrationOptions
     };
 
+    this._optionsManager.wrapEventHandlers(options);
+
     this._instance = new this._WidgetClass(element, options);
     this._optionsManager.setInstance(this._instance);
     this._instance.on("optionChanged", this._optionsManager.handleOptionChange);
   }
 
-  private _updateTemplatesState(callback) {
+  private _updateTemplatesState(callback: any) {
     this.setState((state: IState) => {
         const templates = { ...state.templates };
         callback(templates);
@@ -169,11 +171,6 @@ abstract class ComponentBase<P extends IHtmlOptions> extends React.PureComponent
   private _prepareProps(rawProps: Record<string, any>): IWidgetConfig {
     const separatedProps = separateProps(rawProps, this._defaults, this._templateProps);
 
-    const options: Record<string, any> = {};
-    Object.keys(separatedProps.options).forEach((key) => {
-        options[key] = this._optionsManager.wrapEventHandler(separatedProps.options[key], key);
-    });
-
     const templateOptions = getTemplateOptions({
       templateProps: this._templateProps,
       options: separatedProps.templates,
@@ -190,7 +187,7 @@ abstract class ComponentBase<P extends IHtmlOptions> extends React.PureComponent
     } : undefined;
 
     return {
-        options,
+        options: separatedProps.options,
         defaults: separatedProps.defaults,
         integrationOptions
     };
diff --git a/src/core/options-manager.ts b/src/core/options-manager.ts
index ecc26a2..719ba5a 100644
--- a/src/core/options-manager.ts
+++ b/src/core/options-manager.ts
@@ -28,6 +28,10 @@ interface INestedOptionClass {
     props: object;
 }
 
+function isEventHanlder(optionName: string, optionValue: any) {
+    return optionName.substr(0, 2) === "on" && typeof optionValue === "function";
+}
+
 class OptionsManager {
 
     private readonly _guards: Record<string, number> = {};
@@ -40,6 +44,7 @@ class OptionsManager {
 
     constructor(optionValueGetter: (name: string) => any) {
         this._optionValueGetter = optionValueGetter;
+        this._setOption = this._setOption.bind(this);
         this._registerNestedOption = this._registerNestedOption.bind(this);
 
         this.registerNestedOption = this.registerNestedOption.bind(this);
@@ -57,16 +62,13 @@ class OptionsManager {
         this._instance = instance;
     }
 
-    public wrapEventHandler(optionValue: any, optionName: string): any {
-        if (optionName.substr(0, 2) === "on" && typeof optionValue === "function") {
-            return (...args: any[]) => {
-                if (!this._updatingProps) {
-                    optionValue(...args);
-                }
-            };
-        }
-
-        return optionValue;
+    public wrapEventHandlers(options: Record<string, any>) {
+        Object.keys(options).forEach((name) => {
+            const value = options[name];
+            if (isEventHanlder(name, value)) {
+                options[name] = this._wrapEventHandler(value);
+            }
+        });
     }
 
     public handleOptionChange(e: { name: string, fullName: string, value: any }) {
@@ -125,7 +127,7 @@ class OptionsManager {
                 this._instance.beginUpdate();
                 this._updatingProps = true;
             }
-            this._instance.option(optionName, newProps[optionName]);
+            this._setOption(optionName, newProps[optionName]);
         }
 
         if (this._updatingProps) {
@@ -142,6 +144,22 @@ class OptionsManager {
         return this._registerNestedOption(component, this._nestedOptions);
     }
 
+    private _setOption(name: string, value: any): void {
+        let actualValue = value;
+        if (isEventHanlder(name, value)) {
+            actualValue = this._wrapEventHandler(value);
+        }
+        this._instance.option(name, actualValue);
+    }
+
+    private _wrapEventHandler(handler: any) {
+        return (...args: any[]) => {
+            if (!this._updatingProps) {
+                handler(...args);
+            }
+        };
+    }
+
     private _getNestedOptionsObjects(
         optionsCollection: Record<string, INestedOptionDescr>,
         stateUpdater: any
@@ -245,13 +263,13 @@ class OptionsManager {
         return optionComponent;
     }
 
-    private _setGuard(optionName, optionValue): void {
+    private _setGuard(optionName: string, optionValue: any): void {
         if (this._guards[optionName] !== undefined) {
             return;
         }
 
         const guardId = window.setTimeout(() => {
-            this._instance.option(optionName, optionValue);
+            this._setOption(optionName, optionValue);
             window.clearTimeout(guardId);
             delete this._guards[optionName];
         });


Test patch:
diff --git a/src/core/__tests__/props-updating.test.tsx b/src/core/__tests__/props-updating.test.tsx
index 9e2fe1e..ef30357 100644
--- a/src/core/__tests__/props-updating.test.tsx
+++ b/src/core/__tests__/props-updating.test.tsx
@@ -65,28 +65,82 @@ describe("option control", () => {
         expect(eventHandlers).toHaveProperty("optionChanged");
     });
 
-    it("does not fire events when option changed while props updating", () => {
-        const controlledOptionChanged = jest.fn();
-        const component = shallow(
-            <ControlledComponent controlledOption={"controlled"} onControlledOptionChanged={controlledOptionChanged} />
-        );
-        Widget.option.mockImplementation(
-            (name: string) => {
-                if (name === "controlledOption") {
-                    WidgetClass.mock.calls[0][1].onControlledOptionChanged();
+    describe("handler option", () => {
+
+        it("is not fired when option changed on props updating", () => {
+            const handler = jest.fn();
+            const component = shallow(
+                <ControlledComponent
+                    controlledOption={"controlled"}
+                    onControlledOptionChanged={handler}
+                />
+            );
+            Widget.option.mockImplementation(
+                (name: string) => {
+                    if (name === "controlledOption") {
+                        WidgetClass.mock.calls[0][1].onControlledOptionChanged();
+                    }
                 }
-            }
-        );
-        component.setProps({
-            controlledOption: "changed"
+            );
+            component.setProps({
+                controlledOption: "changed"
+            });
+
+            expect(handler.mock.calls.length).toBe(0);
+
+            Widget.option("controlledOption", "controlled");
+
+            expect(handler.mock.calls.length).toBe(1);
+
         });
 
-        expect(controlledOptionChanged.mock.calls.length).toBe(0);
+        it("is not fired when option changed on props updating (handler updated)", () => {
+            const handler = jest.fn();
+            const component = shallow(
+                <ControlledComponent
+                    controlledOption={"controlled"}
+                    onControlledOptionChanged={jest.fn()}
+                />
+            );
+            Widget.option.mockImplementation(
+                (name: string) => {
+                    if (name === "controlledOption") {
+                        Widget.option.mock.calls[0][1]();
+                    }
+                }
+            );
+
+            component.setProps({
+                onControlledOptionChanged: handler
+            });
+
+            component.setProps({
+                controlledOption: "changed"
+            });
 
-        Widget.option("controlledOption", "controlled");
+            expect(handler.mock.calls.length).toBe(0);
 
-        expect(controlledOptionChanged.mock.calls.length).toBe(1);
+            Widget.option("controlledOption", "controlled");
 
+            expect(handler.mock.calls.length).toBe(1);
+        });
+
+        it("is not updated on other prop updating", () => {
+            const controlledOptionChanged = jest.fn();
+            const component = shallow(
+                <ControlledComponent
+                    anotherOption={"abc"}
+                    onControlledOptionChanged={controlledOptionChanged}
+                />
+            );
+
+            component.setProps({
+                anotherOption: "def"
+            });
+
+            expect(Widget.option.mock.calls.length).toBe(1);
+            expect(Widget.option.mock.calls[0]).toEqual(["anotherOption", "def"]);
+        });
     });
 
     it("rolls option value back", () => {