Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Slider] Fix memory leaks #12537

Merged
merged 1 commit into from
Aug 19, 2018
Merged

[Slider] Fix memory leaks #12537

merged 1 commit into from
Aug 19, 2018

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Aug 15, 2018

Removes 2 potential errors and one that already was showing up sometimes in CI.

Basically #11889 added global event listeners that were not removed when the component was unmounted which could cause leaks or errors if the component is unmounted between mousedown and mouseup.

The timeout related leak was not dangerous but triggers react warnings.

Fixes one error surfaced in #12531

@@ -142,6 +142,10 @@ function addEventListener(node, event, handler, capture) {
},
};
}
// mock return value of addEventListener with noop; used as default
Copy link
Member

@oliviertassinari oliviertassinari Aug 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just refactored the existing pattern but I'll change it.

@oliviertassinari oliviertassinari added the package: lab Specific to @mui/lab label Aug 15, 2018
@eps1lon
Copy link
Member Author

eps1lon commented Aug 15, 2018

I'd like to wait for #12535 to be merged to see if the tests account for this change. They were broken before the test so they currently have little significance to the correctness of this change.

@eps1lon eps1lon force-pushed the fix-slider-leak branch 2 times, most recently from d7a96c7 to ad9af5b Compare August 16, 2018 20:33
@eps1lon
Copy link
Member Author

eps1lon commented Aug 16, 2018

Rebased to resolve unrelated CI fails.

I'm not quite satisfied with the leak tests since they don't check for the timeout which could potentially throw errors in other tests (it was not trivial to provide a custom theme to the component while also being able to test the state of the slider; see enzymejs/enzyme#1289). However I'm confident that we can skip the setTimeout call completely which would make the component easier to test and reason about.

I think that the original author wanted to emulate serial transitions on the thumb (first to left/top then the width) which can be achieved by css transitions alone. Currently the jumped state and transition time don't have the same duration anyway which already grants another look at the behavior.

@mbrookes
Copy link
Member

@eps1lon Great to have someone with your obvious experience working on this! 👍

@eps1lon
Copy link
Member Author

eps1lon commented Aug 19, 2018

Rebased with master. PR is ready for review.

@oliviertassinari oliviertassinari changed the title [Slider] fix memory leaks [Slider] Fix memory leaks Aug 19, 2018
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Aug 19, 2018
@mbrookes mbrookes merged commit 905a961 into mui:master Aug 19, 2018
@mbrookes
Copy link
Member

@eps1lon Great PR. Thanks!

@eps1lon eps1lon deleted the fix-slider-leak branch August 20, 2018 08:36
@oliviertassinari oliviertassinari added component: slider This is the name of the generic UI component, not the React module! and removed package: lab Specific to @mui/lab labels Jan 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants