fix: extra onChange when value out of range (#711)

* fix extra onChange when value out of range

* improve test case

src/Range.tsx

@@ -1,6 +1,5 @@
 import React from 'react';
 import classNames from 'classnames';
-import shallowEqual from 'shallowequal';
 import Track from './common/Track';
 import createSlider from './common/createSlider';
 import * as utils from './utils';
@@ -132,7 +131,9 @@ class Range extends React.Component<RangeProps, RangeState> {
   }
 
   static getDerivedStateFromProps(props, state) {
-    if (!('value' in props || 'min' in props || 'max' in props)) return null;
+    if (!('value' in props || 'min' in props || 'max' in props)) {
+      return null;
+    }
 
     const value = props.value || state.bounds;
     let nextBounds = value.map((v, i) =>
@@ -165,17 +166,13 @@ class Range extends React.Component<RangeProps, RangeState> {
   }
 
   componentDidUpdate(prevProps, prevState) {
-    if (!('value' in this.props || 'min' in this.props || 'max' in this.props)) {
+    const { onChange, value, min, max } = this.props;
+    if (!('min' in this.props || 'max' in this.props)) {
       return;
     }
-    if (
-      this.props.min === prevProps.min &&
-      this.props.max === prevProps.max &&
-      shallowEqual(this.props.value, prevProps.value)
-    ) {
+    if (min === prevProps.min && max === prevProps.max) {
       return;
     }
-    const { onChange, value } = this.props;
     const currentValue = value || prevState.bounds;
     if (currentValue.some(v => utils.isValueOutOfRange(v, this.props))) {
       const newValues = currentValue.map(v => utils.ensureValueInRange(v, this.props));

src/Slider.tsx

@@ -98,19 +98,23 @@ class Slider extends React.Component<SliderProps, SliderState> {
 
   prevMovedHandleIndex: number;
 
-  componentDidUpdate(_: SliderProps, prevState: SliderState) {
-    if (!('value' in this.props || 'min' in this.props || 'max' in this.props)) {
+  componentDidUpdate(prevProps: SliderProps, prevState: SliderState) {
+    const { min, max, value, onChange } = this.props;
+    if (!('min' in this.props || 'max' in this.props)) {
       return;
     }
-    const { value, onChange } = this.props;
     const theValue = value !== undefined ? value : prevState.value;
     const nextValue = this.trimAlignValue(theValue, this.props);
-    if (nextValue !== prevState.value) {
-      // eslint-disable-next-line
-      this.setState({ value: nextValue });
-      if (utils.isValueOutOfRange(theValue, this.props)) {
-        onChange(nextValue);
-      }
+    if (nextValue === prevState.value) {
+      return;
+    }
+    // eslint-disable-next-line
+    this.setState({ value: nextValue });
+    if (
+      !(min === prevProps.min && max === prevProps.max) &&
+      utils.isValueOutOfRange(theValue, this.props)
+    ) {
+      onChange(nextValue);
     }
   }
 

src/common/createSlider.tsx

@@ -232,19 +232,19 @@ export default function createSlider<
     }
 
     focus() {
-      if (!this.props.disabled) {
-        this.handlesRefs[0].focus();
+      if (this.props.disabled) {
+        return;
       }
+      this.handlesRefs[0]?.focus();
     }
 
     blur() {
-      if (!this.props.disabled) {
-        Object.keys(this.handlesRefs).forEach(key => {
-          if (this.handlesRefs[key] && this.handlesRefs[key].blur) {
-            this.handlesRefs[key].blur();
-          }
-        });
+      if (this.props.disabled) {
+        return;
       }
+      Object.keys(this.handlesRefs).forEach(key => {
+        this.handlesRefs[key]?.blur?.();
+      });
     }
 
     calcValue(offset: number) {

tests/common/SliderTooltip.test.js

@@ -1,5 +1,6 @@
 import * as React from 'react';
 import { mount } from 'enzyme';
+import { act } from 'react-dom/test-utils';
 import SliderTooltip from '../../src/common/SliderTooltip';
 
 it('should keepAlign by calling forcePopupAlign', async () => {
@@ -16,6 +17,8 @@ it('should keepAlign by calling forcePopupAlign', async () => {
     </SliderTooltip>,
   );
   ref.forcePopupAlign = jest.fn();
-  await new Promise(res => setTimeout(res, 200));
+  await act(async () => {
+    await new Promise(res => setTimeout(res, 200));
+  });
   expect(ref.forcePopupAlign).toHaveBeenCalled();
 });

tests/common/createSlider.test.js

@@ -77,16 +77,42 @@ describe('createSlider', () => {
     expect(rangeOnChange).toHaveBeenLastCalledWith([10, 10]);
   });
 
+  it('should not trigger onChange when no min and max', () => {
+    const sliderOnChange = jest.fn();
+    const sliderWrapper = mount(<Slider onChange={sliderOnChange} />);
+    sliderWrapper.setProps({ value: 100 });
+    expect(sliderOnChange).not.toHaveBeenCalled();
+
+    const rangeOnChange = jest.fn();
+    const rangeWrapper = mount(<Range onChange={rangeOnChange} />);
+    rangeWrapper.setProps({ value: [0, 100] });
+    expect(rangeOnChange).not.toHaveBeenCalled();
+  });
+
+  it('should not trigger onChange when value is out of range', () => {
+    const sliderOnChange = jest.fn();
+    const sliderWrapper = mount(<Slider value={9} max={10} onChange={sliderOnChange} />);
+    sliderWrapper.setProps({ value: 11 });
+    expect(sliderWrapper.state('value')).toBe(10);
+    expect(sliderOnChange).not.toHaveBeenCalled();
+
+    const rangeOnChange = jest.fn();
+    const rangeWrapper = mount(<Range max={10} onChange={rangeOnChange} />);
+    rangeWrapper.setProps({ value: [0, 100] });
+    expect(rangeWrapper.state('bounds')).toEqual([0, 10]);
+    expect(rangeOnChange).not.toHaveBeenCalled();
+  });
+
   it('should not call onChange when value is the same', () => {
     const handler = jest.fn();
 
-    const sliderWrapper = mount(<Slider onChange={handler}/>);
+    const sliderWrapper = mount(<Slider onChange={handler} />);
     const sliderHandleWrapper = sliderWrapper.find('.rc-slider-handle').at(1);
     sliderHandleWrapper.simulate('mousedown');
     sliderHandleWrapper.simulate('mousemove');
     sliderHandleWrapper.simulate('mouseup');
 
-    const rangeWrapper = mount(<Range onChange={handler}/>);
+    const rangeWrapper = mount(<Range onChange={handler} />);
     const rangeHandleWrapper = rangeWrapper.find('.rc-slider-handle-1').at(1);
     rangeHandleWrapper.simulate('mousedown');
     rangeHandleWrapper.simulate('mousemove');
@@ -117,11 +143,15 @@ describe('createSlider', () => {
   it('should set `dragOffset` to correct value when the left handle is clicked off-center', () => {
     const wrapper = mount(<Slider />);
     setWidth(wrapper.instance().sliderRef, 100);
-    const leftHandle = wrapper.find('.rc-slider-handle').at(1).instance();
+    const leftHandle = wrapper
+      .find('.rc-slider-handle')
+      .at(1)
+      .instance();
     wrapper.simulate('mousedown', {
       type: 'mousedown',
       target: leftHandle,
-      pageX: 5, button: 0,
+      pageX: 5,
+      button: 0,
       stopPropagation() {},
       preventDefault() {},
     });
@@ -131,19 +161,25 @@ describe('createSlider', () => {
   it('should respect `dragOffset` while dragging the handle via MouseEvents', () => {
     const wrapper = mount(<Slider />);
     setWidth(wrapper.instance().sliderRef, 100);
-    const leftHandle = wrapper.find('.rc-slider-handle').at(1).instance();
+    const leftHandle = wrapper
+      .find('.rc-slider-handle')
+      .at(1)
+      .instance();
     wrapper.simulate('mousedown', {
       type: 'mousedown',
       target: leftHandle,
-      pageX: 5, button: 0,
+      pageX: 5,
+      button: 0,
       stopPropagation() {},
       preventDefault() {},
     });
     expect(wrapper.instance().dragOffset).toBe(5);
-    wrapper.instance().onMouseMove({ // to propagation
+    wrapper.instance().onMouseMove({
+      // to propagation
       type: 'mousemove',
       target: leftHandle,
-      pageX: 14, button: 0,
+      pageX: 14,
+      button: 0,
       stopPropagation() {},
       preventDefault() {},
     });
@@ -153,33 +189,40 @@ describe('createSlider', () => {
   it('should not go to right direction when mouse go to the left', () => {
     const wrapper = mount(<Slider />);
     setWidth(wrapper.instance().sliderRef, 100);
-    const leftHandle = wrapper.find('.rc-slider-handle').at(1).instance();
+    const leftHandle = wrapper
+      .find('.rc-slider-handle')
+      .at(1)
+      .instance();
     wrapper.simulate('mousedown', {
       type: 'mousedown',
       target: leftHandle,
-      pageX: 5, button: 0,
+      pageX: 5,
+      button: 0,
       stopPropagation() {},
       preventDefault() {},
     });
     expect(wrapper.instance().getValue()).toBe(0); // zero on start
-    wrapper.instance().onMouseMove({ // to propagation
+    wrapper.instance().onMouseMove({
+      // to propagation
       type: 'mousemove',
       target: leftHandle,
-      pageX: 0, button: 0,
+      pageX: 0,
+      button: 0,
       stopPropagation() {},
       preventDefault() {},
     });
     expect(wrapper.instance().getValue()).toBe(0); // still zero
   });
 
-  it('should set `dragOffset` to 0 when the MouseEvent target isn\'t a handle', () => {
+  it("should set `dragOffset` to 0 when the MouseEvent target isn't a handle", () => {
     const wrapper = mount(<Slider />);
     setWidth(wrapper.instance().sliderRef, 100);
     const sliderTrack = wrapper.find('.rc-slider-track').get(0);
     wrapper.simulate('mousedown', {
       type: 'mousedown',
       target: sliderTrack,
-      pageX: 5, button: 0,
+      pageX: 5,
+      button: 0,
       stopPropagation() {},
       preventDefault() {},
     });
@@ -189,7 +232,10 @@ describe('createSlider', () => {
   it('should set `dragOffset` to correct value when the left handle is touched off-center', () => {
     const wrapper = mount(<Slider />);
     setWidth(wrapper.instance().sliderRef, 100);
-    const leftHandle = wrapper.find('.rc-slider-handle').at(1).instance();
+    const leftHandle = wrapper
+      .find('.rc-slider-handle')
+      .at(1)
+      .instance();
     wrapper.simulate('touchstart', {
       type: 'touchstart',
       target: leftHandle,
@@ -203,7 +249,10 @@ describe('createSlider', () => {
   it('should respect `dragOffset` while dragging the handle via TouchEvents', () => {
     const wrapper = mount(<Slider />);
     setWidth(wrapper.instance().sliderRef, 100);
-    const leftHandle = wrapper.find('.rc-slider-handle').at(1).instance();
+    const leftHandle = wrapper
+      .find('.rc-slider-handle')
+      .at(1)
+      .instance();
     wrapper.simulate('touchstart', {
       type: 'touchstart',
       target: leftHandle,
@@ -212,7 +261,8 @@ describe('createSlider', () => {
       preventDefault() {},
     });
     expect(wrapper.instance().dragOffset).toBe(5);
-    wrapper.instance().onTouchMove({ // to propagation
+    wrapper.instance().onTouchMove({
+      // to propagation
       type: 'touchmove',
       target: leftHandle,
       touches: [{ pageX: 14 }],
@@ -222,7 +272,7 @@ describe('createSlider', () => {
     expect(wrapper.instance().getValue()).toBe(9);
   });
 
-  it('should set `dragOffset` to 0 when the TouchEvent target isn\'t a handle', () => {
+  it("should set `dragOffset` to 0 when the TouchEvent target isn't a handle", () => {
     const wrapper = mount(<Slider />);
     setWidth(wrapper.instance().sliderRef, 100);
     const sliderTrack = wrapper.find('.rc-slider-track').get(0);
@@ -240,18 +290,22 @@ describe('createSlider', () => {
     const labelId = 'to-be-clicked';
     const marks = {
       0: 'some other label',
-      100: <span id={labelId}>some label</span>
+      100: <span id={labelId}>some label</span>,
     };
 
     const sliderOnAfterChange = jest.fn();
-    const sliderWrapper = mount(<Slider value={0} marks={marks} onAfterChange={sliderOnAfterChange} />);
+    const sliderWrapper = mount(
+      <Slider value={0} marks={marks} onAfterChange={sliderOnAfterChange} />,
+    );
     const sliderHandleWrapper = sliderWrapper.find(`#${labelId}`).at(0);
     sliderHandleWrapper.simulate('mousedown');
     sliderHandleWrapper.simulate('mouseup');
     expect(sliderOnAfterChange).toHaveBeenCalled();
 
     const rangeOnAfterChange = jest.fn();
-    const rangeWrapper = mount(<Range value={[0, 1]} marks={marks} onAfterChange={rangeOnAfterChange} />);
+    const rangeWrapper = mount(
+      <Range value={[0, 1]} marks={marks} onAfterChange={rangeOnAfterChange} />,
+    );
     const rangeHandleWrapper = rangeWrapper.find(`#${labelId}`).at(0);
     rangeHandleWrapper.simulate('mousedown');
     rangeHandleWrapper.simulate('mouseup');
@@ -278,7 +332,10 @@ describe('createSlider', () => {
     const tooltipParent = document.createElement('div');
     tooltipParent.setAttribute('id', 'tooltip');
     const wrapper = mount(
-      <SliderWithTooltip tipProps={tooltipPrefixer} getTooltipContainer={() => document.getElementById('tooltip')} />
+      <SliderWithTooltip
+        tipProps={tooltipPrefixer}
+        getTooltipContainer={() => document.getElementById('tooltip')}
+      />,
     );
     expect(wrapper.instance().props.getTooltipContainer).toBeTruthy();
   });