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 doesn't work with frozen arrays #27347

Closed
PinkaminaDianePie opened this issue Jul 18, 2021 · 7 comments
Closed

Slider doesn't work with frozen arrays #27347

PinkaminaDianePie opened this issue Jul 18, 2021 · 7 comments
Labels
bug 🐛 Something doesn't work component: slider This is the name of the generic UI component, not the React module!

Comments

@PinkaminaDianePie
Copy link

Because of this perf optimization Slider doesn't work with frozen arrays:
https://github.com/mui-org/material-ui/blob/a3dbd6cc4e3f07df702a807ffb3f058586cff324/packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js#L96
If I pass a frozen array as a value prop to the slider, it throws an error because it attempts to sort the result of the setValueIndex function, but it's impossible to sort the frozen array. Currently, I receive a frozen array from the Apollo. I can spread/slice manually, before passing to the Slider, but it feels dirty to do it. It's probably just better to drop this optimization - copying of such a small array is not expensive

@eps1lon
Copy link
Member

eps1lon commented Jul 19, 2021

It doesn't look like this bug report has enough info for one of us to reproduce it.

Please provide a CodeSandbox (https://material-ui.com/r/issue-template-next), a link to a repository on GitHub, or provide a minimal code example that reproduces the problem.

Here are some tips for providing a minimal example: https://stackoverflow.com/help/mcve

@eps1lon eps1lon added component: slider This is the name of the generic UI component, not the React module! status: waiting for author Issue with insufficient information labels Jul 19, 2021
@PinkaminaDianePie
Copy link
Author

sure, here is it: https://codesandbox.io/s/damp-glade-ld9m7?file=/src/Demo.tsx:134-148
try to drag the slider, you will get an error

@eps1lon eps1lon added bug 🐛 Something doesn't work and removed status: waiting for author Issue with insufficient information labels Jul 19, 2021
@eps1lon
Copy link
Member

eps1lon commented Jul 19, 2021

Thanks for the report!

Input parameters should be readonly. Object.freeze shows that this is currently not the case, which is a bug.

@KyLoc20
Copy link

KyLoc20 commented Jul 19, 2021

I think the type checker is not strict enough.
https://github.com/mui-org/material-ui/blob/014dce1122943666f45f44844ef5067beb455c4a/packages/material-ui/src/Slider/Slider.js#L686

PropTypes.arrayOf(PropTypes.number) allows immutable objects which are generated from Object.freeze() or Object.seal().
I checked facebook/prop-types#117 (comment) where they suggested using a a custom propType to avoid the situations above.

@eps1lon as you said input params value should be readonly. I wrote a custom propType below.

const isImmutable = (obj) =>
  !Object.isExtensible(obj) || Object.isSealed(obj) || Object.isFrozen(obj);

const mutableArrayOf = function (innerTypes) {
  function validate(props, propName, componentName) {
    const arr = props[propName];
    /**
     * check array and inner elements here.
     */
    PropTypes.checkPropTypes(
      { array: PropTypes.arrayOf(innerTypes) },
      { array: arr },
      `prop`,
      `${componentName}.${propName}`
    );
    /**
     * check mutable here.
     */
    if (isImmutable(arr)) {
      return new Error(
        `Invalid prop [${arr}] supplied to ${componentName}. Validation failed due to this is immutable.`
      );
    }
  }
  return validate;
};
Slider.propTypes = {
  value: PropTypes.oneOfType([PropTypes.number,mutableArrayOf(PropTypes.number)]),
};

@PinkaminaDianePie

This comment has been minimized.

@eps1lon

This comment has been minimized.

@eps1lon
Copy link
Member

eps1lon commented Oct 2, 2021

Fixed in #28472

@eps1lon eps1lon closed this as completed Oct 2, 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

No branches or pull requests

3 participants