[base-ui][useSlider] Suggestions for refactoring the useSlider hook

[pixelass]

Duplicates

• I have searched the existing issues

Latest version

• I have tested the latest version

Steps to reproduce 🕹

Link to live example: https://github.com/mui/material-ui/blob/master/packages/mui-base/src/useSlider/useSlider.ts

Steps:

1. look at code
2. follow your nose
3. it smells
4. understand that I'm trying to lighten the mood with these repro steps

Current behavior 😯

The implementation of useSlider seems overengineered and has several potential issues and code smells.

1. use of non strict equality operators == & != where a strict variant would do the same and add stability. (i.e. here, here or here)
2. passive event listening on events where it isn't needed (i.e. here)
3. no passive event listening on events where it is needed (i.e. here or here
4. Several utility functions like clamp, valueToPercent and others that should probably go into a shared utils file (if not already available) to prevent code duplication.

Addition to 3. & 4.

Passive event listeners should be used when user interaction happens in fast steps (like scrolling, moving pointers, resizing etc.) which could result in jank. removeEventListener doesn't even allow {passive: true} so this is actually invalid code: https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener#parameters

Expected behavior 🤔

Please understand that I have the highest respect for anyone working on this amazing library. I've been using mui libraries for several years and love it a lot.

I expect the code to meet common/best practice and allow developers to understand the behavior. Usually this is achieved by "separation of concerns".

I also expect issues like these to be caught by linters or code-reviews.

Context 🔦

Please understand that I have the highest respect for anyone working on this amazing library. I've been using mui libraries for several years and love it a lot.

I stumbled upon this while trying to debug #39362.
I soon realized that it is close to impossible to get through this code (for me).
Not a single useful comment explaining the decisions or reason for certain steps in the process (except for a nice elaboration on the iOS bug which has been resolved several Major versions ago)

Your environment 🌎

npx @mui/envinfo

  System:
    OS: Windows 10 10.0.22621
  Binaries:
    Node: 18.5.0 - C:\Program Files\nodejs\node.EXE
    Yarn: Not Found
    npm: 8.12.1 - C:\Program Files\nodejs\npm.CMD
  Browsers:
    Chrome: Not Found
    Edge: Chromium (117.0.2045.60)
  npmPackages:
    @emotion/react: 11.11.1 => 11.11.1
    @mui/core-downloads-tracker:  5.14.11
    @mui/icons-material: 5.14.11 => 5.14.11
    @mui/joy: 5.0.0-beta.8 => 5.0.0-beta.8
    @mui/material: ^5.14.11 => 5.14.11
    @mui/private-theming:  5.14.11
    @mui/styled-engine:  5.14.11
    @mui/system:  5.14.11
    @mui/types:  7.2.4
    @mui/utils:  5.14.11
    @types/react: ^18.2.23 => 18.2.23
    react: 18.2.0 => 18.2.0
    react-dom: 18.2.0 => 18.2.0
    typescript: ^5.2.2 => 5.2.2
    @emotion/styled: 11.11.0 => 11.11.0
    @mui/base:  5.0.0-beta.17
    @mui/core-downloads-tracker:  5.14.12
    @mui/icons-material: 5.14.11 => 5.14.11
    @mui/joy: 5.0.0-beta.9 => 5.0.0-beta.9
    @mui/material: ^5.14.11 => 5.14.11
    @mui/private-theming:  5.14.12
    @mui/styled-engine:  5.14.12
    @mui/system:  5.14.12
    @mui/types:  7.2.5
    @mui/utils:  5.14.12
    @types/react: ^18.2.23 => 18.2.23
    react: 18.2.0 => 18.2.0
    react-dom: 18.2.0 => 18.2.0
    typescript: ^5.2.2 => 5.2.2

[pixelass]

clamp implementations:

• material-ui/packages/mui-material/src/Rating/Rating.js

  Lines 21 to 29 in 53c792d

  	function clamp(value, min, max) {
  	if (value < min) {
  	return min;
  	}
  	if (value > max) {
  	return max;
  	}
  	return value;
  	}

• material-ui/packages/mui-base/src/useSlider/useSlider.ts

  Lines 28 to 33 in 53c792d

  	function clamp(value: number, min: number, max: number) {
  	if (value == null) {
  	return min;
  	}
  	return Math.min(Math.max(min, value), max);
  	}

• material-ui/packages/mui-material/src/SpeedDial/SpeedDial.js

  Lines 40 to 48 in f3da5be

  	function clamp(value, min, max) {
  	if (value < min) {
  	return min;
  	}
  	if (value > max) {
  	return max;
  	}
  	return value;
  	}

There's already a pretty flexible clamp implementation including a simpleClamp here: https://github.com/mui/material-ui/blob/master/packages/mui-base/src/unstable_useNumberInput/utils.ts#L9

Yeah probably not the biggest issue but might be something to think about. A lot of these could probably be taken from other libraries or moved into a separate package.
I am using clamp as an example. The same behavior can be seen or numerous other common utility functions.

[michaldudak]

Thanks for your input! Corrections and improvements are always welcome.

use of non strict equality operators == & != where a strict variant would do the same and add stability.

We only use non-strict equality comparison when testing for null (to catch cases where an undefined may appear instead). We enforce this with an ESLint rule. I wouldn't change it.

passive event listening on events where it isn't needed
no passive event listening on events where it is needed

Nice catch! Could you submit a PR that corrects it?

Several utility functions like clamp, valueToPercent and others that should probably go into a shared utils file

Feel free to create a PR that moves them to the @mui/utils package.

[pixelass]

We only use non-strict equality comparison when testing for null (to catch cases where an undefined may appear instead). We enforce this with an ESLint rule. I wouldn't change it.

Thank you for the clarification

One could make use of: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Nullish_coalescing

which is equally short and adjustable to various styles: (but your solution of enforcing it via eslint is exemplary)

const value = getValue() // could retrun any
if (value ?? true) {
  console.log(":poop:");
}
if (value ?? 1) {
  console.log(":poop:");
}

if (value ?? !0) {
  console.log(":poop:");
}

if (undefined ?? true) {
    console.log("undefined")
}
if (null ?? true) {
    console.log("null")
}
if (false ?? true) {
    console.log("false")
}

if (0 ?? true) {
    console.log("0")
}

undefined
null

Nice catch! Could you submit a PR that corrects it?

Sure, I can look into that, since this is straight forward

Feel free to create a PR that moves them to the @mui/utils package.

Sadly not any time soon, if this is still open around end of year I might revisit this.

[Kamino0]

Created PR for useSlider refactoring
I think for utility function moving better create another PR?

[Kamino0]

valueToPercent is used only with the slider, so I don’t see any point in putting it into @mui/utils

[ZeeshanTamboli]

Closing this issue since it has been resolved by #40235 and #40267.