From 47732b8a1486b8a1332cff4f54920e136a8e368b Mon Sep 17 00:00:00 2001
From: Kermit <kermitlx@outlook.com>
Date: Fri, 29 Jan 2021 13:22:42 +0800
Subject: [PATCH] refactor: optimize calling sequence of onChange and
 onSelectAll in rowSelection of Table (#29079)

* chore: optimize order of onChange and onSelect in rowSelection of Table

* chore: add test case

* chore: trigger ci

* ci: trigger
---
 .../__tests__/Table.rowSelection.test.js      | 64 +++++++++++++++++--
 components/table/hooks/useSelection.tsx       | 12 ++--
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/components/table/__tests__/Table.rowSelection.test.js b/components/table/__tests__/Table.rowSelection.test.js
index 9c356d1a1a..58158dad73 100644
--- a/components/table/__tests__/Table.rowSelection.test.js
+++ b/components/table/__tests__/Table.rowSelection.test.js
@@ -178,8 +178,13 @@ describe('Table.rowSelection', () => {
   });
 
   it('fires change & select events', () => {
-    const handleChange = jest.fn();
-    const handleSelect = jest.fn();
+    const order = [];
+    const handleChange = jest.fn().mockImplementation(() => {
+      order.push('onChange');
+    });
+    const handleSelect = jest.fn().mockImplementation(() => {
+      order.push('onSelect');
+    });
     const rowSelection = {
       onChange: handleChange,
       onSelect: handleSelect,
@@ -197,12 +202,22 @@ describe('Table.rowSelection', () => {
     expect(handleSelect.mock.calls[0][1]).toEqual(true);
     expect(handleSelect.mock.calls[0][2]).toEqual([{ key: 3, name: 'Jerry' }]);
     expect(handleSelect.mock.calls[0][3].type).toBe('change');
+    expect(order).toEqual(['onSelect', 'onChange']);
   });
 
   it('fires selectMulti event', () => {
-    const handleSelectMulti = jest.fn();
-    const handleSelect = jest.fn();
+    const order = [];
+    const handleSelectMulti = jest.fn().mockImplementation(() => {
+      order.push('onSelectMultiple');
+    });
+    const handleSelect = jest.fn().mockImplementation(() => {
+      order.push('onSelect');
+    });
+    const handleChange = jest.fn().mockImplementation(() => {
+      order.push('onChange');
+    });
     const rowSelection = {
+      onChange: handleChange,
       onSelect: handleSelect,
       onSelectMultiple: handleSelectMulti,
     };
@@ -238,11 +253,27 @@ describe('Table.rowSelection', () => {
         nativeEvent: { shiftKey: true },
       });
     expect(handleSelectMulti).toHaveBeenCalledWith(false, [], [data[0], data[1], data[2]]);
+
+    expect(order).toEqual([
+      'onSelect',
+      'onChange',
+      'onSelectMultiple',
+      'onChange',
+      'onSelectMultiple',
+      'onChange',
+    ]);
   });
 
   it('fires selectAll event', () => {
-    const handleSelectAll = jest.fn();
+    const order = [];
+    const handleSelectAll = jest.fn().mockImplementation(() => {
+      order.push('onSelectAll');
+    });
+    const handleChange = jest.fn().mockImplementation(() => {
+      order.push('onChange');
+    });
     const rowSelection = {
+      onChange: handleChange,
       onSelectAll: handleSelectAll,
     };
     const wrapper = mount(createTable({ rowSelection }));
@@ -253,6 +284,8 @@ describe('Table.rowSelection', () => {
       .simulate('change', { target: { checked: true } });
     expect(handleSelectAll).toHaveBeenCalledWith(true, data, data);
 
+    expect(order).toEqual(['onSelectAll', 'onChange']);
+
     wrapper
       .find('input')
       .first()
@@ -283,8 +316,15 @@ describe('Table.rowSelection', () => {
   });
 
   it('fires selectInvert event', () => {
-    const handleSelectInvert = jest.fn();
+    const order = [];
+    const handleSelectInvert = jest.fn().mockImplementation(() => {
+      order.push('onSelectInvert');
+    });
+    const handleChange = jest.fn().mockImplementation(() => {
+      order.push('onChange');
+    });
     const rowSelection = {
+      onChange: handleChange,
       onSelectInvert: handleSelectInvert,
       selections: true,
     };
@@ -297,11 +337,20 @@ describe('Table.rowSelection', () => {
     dropdownWrapper.find('.ant-dropdown-menu-item').at(1).simulate('click');
 
     expect(handleSelectInvert).toHaveBeenCalledWith([1, 2, 3]);
+
+    expect(order).toEqual(['onChange', 'onSelectInvert', 'onChange']);
   });
 
   it('fires selectNone event', () => {
-    const handleSelectNone = jest.fn();
+    const order = [];
+    const handleChange = jest.fn().mockImplementation(() => {
+      order.push('onChange');
+    });
+    const handleSelectNone = jest.fn().mockImplementation(() => {
+      order.push('onSelectNone');
+    });
     const rowSelection = {
+      onChange: handleChange,
       onSelectNone: handleSelectNone,
       selections: true,
     };
@@ -314,6 +363,7 @@ describe('Table.rowSelection', () => {
     dropdownWrapper.find('.ant-dropdown-menu-item').last().simulate('click');
 
     expect(handleSelectNone).toHaveBeenCalled();
+    expect(order).toEqual(['onChange', 'onSelectNone', 'onChange']);
   });
 
   it('fires selection event', () => {
diff --git a/components/table/hooks/useSelection.tsx b/components/table/hooks/useSelection.tsx
index 17cfdb9c81..38a7000ddd 100644
--- a/components/table/hooks/useSelection.tsx
+++ b/components/table/hooks/useSelection.tsx
@@ -287,7 +287,6 @@ export default function useSelection<RecordType>(
             });
 
             const keys = Array.from(keySet);
-            setSelectedKeys(keys);
             if (onSelectInvert) {
               devWarning(
                 false,
@@ -296,6 +295,8 @@ export default function useSelection<RecordType>(
               );
               onSelectInvert(keys);
             }
+
+            setSelectedKeys(keys);
           },
         };
       }
@@ -304,10 +305,11 @@ export default function useSelection<RecordType>(
           key: 'none',
           text: tableLocale.selectNone,
           onSelect() {
-            setSelectedKeys([]);
             if (onSelectNone) {
               onSelectNone();
             }
+
+            setSelectedKeys([]);
           },
         };
       }
@@ -350,7 +352,6 @@ export default function useSelection<RecordType>(
         }
 
         const keys = Array.from(keySet);
-        setSelectedKeys(keys);
 
         if (onSelectAll) {
           onSelectAll(
@@ -359,6 +360,8 @@ export default function useSelection<RecordType>(
             changeKeys.map(k => getRecordByKey(k)),
           );
         }
+
+        setSelectedKeys(keys);
       };
 
       // ===================== Render =====================
@@ -516,7 +519,6 @@ export default function useSelection<RecordType>(
                     }
 
                     const keys = Array.from(keySet);
-                    setSelectedKeys(keys);
                     if (onSelectMultiple) {
                       onSelectMultiple(
                         !checked,
@@ -524,6 +526,8 @@ export default function useSelection<RecordType>(
                         changedKeys.map(recordKey => getRecordByKey(recordKey)),
                       );
                     }
+
+                    setSelectedKeys(keys);
                   } else {
                     // Single record selected
                     const originCheckedKeys = derivedSelectedKeys;