Skip to content

Commit

Permalink
[Slider] fix memory leaks
Browse files Browse the repository at this point in the history
  • Loading branch information
eps1lon committed Aug 18, 2018
1 parent a29c7f1 commit e92b1c6
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 23 deletions.
32 changes: 12 additions & 20 deletions packages/material-ui-lab/src/Slider/Slider.js
Original file line number Diff line number Diff line change
Expand Up @@ -134,15 +134,6 @@ export const styles = theme => {
};
};

function addEventListener(node, event, handler, capture) {
node.addEventListener(event, handler, capture);
return {
remove: function remove() {
node.removeEventListener(event, handler, capture);
},
};
}

function percentToValue(percent, min, max) {
return ((max - min) * percent) / 100 + min;
}
Expand Down Expand Up @@ -198,6 +189,8 @@ if (process.env.NODE_ENV !== 'production' && !React.createContext) {
class Slider extends React.Component {
state = { currentState: 'initial' };

jumpAnimationTimeoutId = -1;

componentDidMount() {
if (this.containerRef) {
this.containerRef.addEventListener('touchstart', preventPageScrolling, { passive: false });
Expand All @@ -206,6 +199,9 @@ class Slider extends React.Component {

componentWillUnmount() {
this.containerRef.removeEventListener('touchstart', preventPageScrolling, { passive: false });
document.body.removeEventListener('mousemove', this.handleMouseMove);
document.body.removeEventListener('mouseup', this.handleMouseUp);
clearTimeout(this.jumpAnimationTimeoutId);
}

static getDerivedStateFromProps(nextProps, prevState) {
Expand Down Expand Up @@ -281,7 +277,7 @@ class Slider extends React.Component {
event.preventDefault();
this.setState({ currentState: 'activated' });

this.globalMouseUpListener = addEventListener(document, 'touchend', this.handleMouseUp);
document.body.addEventListener('touchend', this.handleMouseUp);

if (typeof this.props.onDragStart === 'function') {
this.props.onDragStart(event);
Expand All @@ -292,8 +288,8 @@ class Slider extends React.Component {
event.preventDefault();
this.setState({ currentState: 'activated' });

this.globalMouseUpListener = addEventListener(document, 'mouseup', this.handleMouseUp);
this.globalMouseMoveListener = addEventListener(document, 'mousemove', this.handleMouseMove);
document.body.addEventListener('mousemove', this.handleMouseMove);
document.body.addEventListener('mouseup', this.handleMouseUp);

if (typeof this.props.onDragStart === 'function') {
this.props.onDragStart(event);
Expand All @@ -303,13 +299,8 @@ class Slider extends React.Component {
handleMouseUp = event => {
this.setState({ currentState: 'normal' });

if (this.globalMouseUpListener) {
this.globalMouseUpListener.remove();
}

if (this.globalMouseMoveListener) {
this.globalMouseMoveListener.remove();
}
document.body.removeEventListener('mousemove', this.handleMouseMove);
document.body.removeEventListener('mouseup', this.handleMouseUp);

if (typeof this.props.onDragEnd === 'function') {
this.props.onDragEnd(event);
Expand Down Expand Up @@ -373,7 +364,8 @@ class Slider extends React.Component {

playJumpAnimation() {
this.setState({ currentState: 'jumped' }, () => {
setTimeout(() => {
clearTimeout(this.jumpAnimationTimeoutId);
this.jumpAnimationTimeoutId = setTimeout(() => {
this.setState({ currentState: 'normal' });
}, this.props.theme.transitions.duration.complex);
});
Expand Down
34 changes: 31 additions & 3 deletions packages/material-ui-lab/src/Slider/Slider.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,43 @@ describe('<Slider />', () => {
wrapper.simulate('click');
wrapper.simulate('mousedown');
// document.simulate('mouseup')
const mouseupEvent = document.createEvent('HTMLEvents');
mouseupEvent.initEvent('mouseup', false, true);
document.dispatchEvent(mouseupEvent);
document.body.dispatchEvent(new window.MouseEvent('mouseup'));

assert.strictEqual(handleChange.callCount, 1, 'should have called the handleChange cb');
assert.strictEqual(handleDragStart.callCount, 1, 'should have called the handleDragStart cb');
assert.strictEqual(handleDragEnd.callCount, 1, 'should have called the handleDragEnd cb');
});

describe('unmount', () => {
it('should not have global event listeners registered after unmount', () => {
const handleChange = spy();
const handleDragEnd = spy();

const wrapper = mount(<Slider onChange={handleChange} onDragEnd={handleDragEnd} value={0} />);

const callGlobalListeners = () => {
document.body.dispatchEvent(new window.MouseEvent('mousemove'));
document.body.dispatchEvent(new window.MouseEvent('mouseup'));
};

wrapper.simulate('mousedown');
callGlobalListeners();
// pre condition: the dispatched event actually did something when mounted
assert.strictEqual(handleChange.callCount, 1, 'should have called the handleChange cb');
assert.strictEqual(handleDragEnd.callCount, 1, 'should have called the handleDragEnd cb');

wrapper.unmount();

// After unmounting global listeners should not be registered aynmore since that would
// break component encapsulation. If they are still mounted either react will throw warnings
// or other component logic throws.
// post condition: the dispatched events dont cause errors/warnings
callGlobalListeners();
assert.strictEqual(handleChange.callCount, 1, 'should not have called handleChange again');
assert.strictEqual(handleDragEnd.callCount, 1, 'should not have called handleDragEnd again');
});
});

describe('prop: vertical', () => {
it('should render with the default and vertical classes', () => {
const wrapper = shallow(<Slider vertical value={0} />);
Expand Down

0 comments on commit e92b1c6

Please sign in to comment.