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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[pickers] shouldDisableDate is called a lot of time for each date #6787

Closed
2 tasks done
danhab99 opened this issue Nov 8, 2022 · 6 comments 路 Fixed by #10502
Closed
2 tasks done

[pickers] shouldDisableDate is called a lot of time for each date #6787

danhab99 opened this issue Nov 8, 2022 · 6 comments 路 Fixed by #10502
Labels
component: pickers This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation

Comments

@danhab99
Copy link

danhab99 commented Nov 8, 2022

Duplicates

  • I have searched the existing issues

Latest version

  • I have tested the latest version

Steps to reproduce 馃暪

import {
  StaticDatePicker,
} from "@mui/x-date-pickers";
import { default as moment, Moment } from "moment";

  function MyPicker() {
    const avaliableDates: Moment[] = useAvaliableDates();

    const minDate = useMemo(() => {
      return moment.min(avaliableDates);
    }, [avaliableDates]);

    const maxDate = useMemo(() => {
      return moment.max(avaliableDates);
    }, [avaliableDates]);

    const shouldDisableDate = useCallback(
      function shouldDisableDateFunc(day: Moment) {
        if (avaliableDates) {
          for (const a of avaliableDates) {
            const x = day.isSame(a);
            if (x) {
              return false;
            }
          }
        }
        console.warn("no avaliable dates");
        return true;
      },
      [avaliableDates]
    );
    return (
      <StaticDatePicker
        displayStaticWrapperAs="desktop"
        disableFuture={true}
        reduceAnimations={true}
        openTo="day"
        value={dateObj}
        onChange={handleOnDateChange}
        renderInput={(params) => <></>}
        disableMaskedInput
        shouldDisableDate={shouldDisableDate}
        minDate={minDate}
        maxDate={maxDate}
      />
    );
  }

Current behavior 馃槸

When avaliableDates is defined the StaticDatePicker will call shouldDisableDate between 5-6 times for every avaliableDate (avaliableDate.length * (5.5 +- 0.5)). shouldDisableDateFunc takes ~3-5ms to run so that's not where the bottle neck is.

With avaliableDates having 70 items, the StaticDatePicker calls shouldDisableDateFunc 430 time (tested with chrome profiler) and takes >1000ms to become visible.

Expected behavior 馃

StaticDatePicker should render in less than 200ms (or as good as instant from the perspective of the user)

Context 馃敠

Need to disable dates so that the user won't pick them

Your environment 馃寧

npx @mui/envinfo
  System:
    OS: Linux 5.10 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish)
  Binaries:
    Node: 18.10.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.19.2 - /usr/local/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: Not Found
  npmPackages:
    @emotion/react: ^11.9.3 => 11.10.4 
    @emotion/styled: ^11.9.3 => 11.10.4 
    @mui/base:  5.0.0-alpha.101 
    @mui/core-downloads-tracker:  5.10.9 
    @mui/icons-material: ^5.8.4 => 5.10.9 
    @mui/material: ^5.8.7 => 5.10.9 
    @mui/private-theming:  5.10.9 
    @mui/styled-engine:  5.10.8 
    @mui/system:  5.10.9 
    @mui/types:  7.2.0 
    @mui/utils:  5.10.9 
    @mui/x-date-pickers: ^5.0.0-beta.5 => 5.0.4 
    @types/react: 18.0.14 => 18.0.14 
    react: 18.2.0 => 18.2.0 
    react-dom: 18.2.0 => 18.2.0 
    typescript: ^4.8.2 => 4.8.4 

Order ID 馃挸 (optional)

No response

@danhab99 danhab99 added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 8, 2022
@flaviendelangle
Copy link
Member

shouldDisableDateFunc takes ~3-5ms to run so that's not where the bottle neck is.

What makes this function so slow on your side ? Could you speed it up ? For instance by avoiding linear complexity with object instead of lists to check the availability of a date.

Maybe we can improve the amount of calls, but it will probably not solve the root issue if your method takes several ms per date.

@flaviendelangle flaviendelangle added component: pickers This is the name of the generic UI component, not the React module! status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 9, 2022
@flaviendelangle flaviendelangle changed the title StaticDatePicker experiences excessive render time when prop shouldDisableDate is included [pickers] shouldDisableDate is called a lot of time for each date Nov 9, 2022
@danhab99
Copy link
Author

Could you speed it up ?

Yeah I managed to speed it up with a binary search, plus including minDate and maxDate helped, even still the component is noticeably sluggish. Perhaps it would make more sense to pass a prop to the StaticDatePicker that is an array if Moments that should be disabled, or maybe the component should only call the shouldDisableDate function only for each date displayed?

In any case this seems like a more process intensive method of doing this.

@github-actions github-actions bot removed the status: waiting for author Issue with insufficient information label Nov 10, 2022
@flaviendelangle
Copy link
Member

flaviendelangle commented Nov 10, 2022

maybe the component should only call the shouldDisableDate function only for each date displayed?

For that part I do agree
We had the issue #4705 that seems to be related and has been fixed recently

@flaviendelangle flaviendelangle added the docs Improvements or additions to the documentation label Dec 13, 2022
@flaviendelangle flaviendelangle self-assigned this Dec 19, 2022
@flaviendelangle flaviendelangle removed their assignment Mar 30, 2023
@michelengelen
Copy link
Member

It seems as if this is not happening anymore (codesandbox with test here).

Any objectiosn closing this @flaviendelangle ?

@LukasTy
Copy link
Member

LukasTy commented Sep 27, 2023

If my silly way of testing is to be believed, then yes, it seems to have improved, the callback is being called almost half as much as on 5.0.4.
Screenshot 2023-09-28 at 00 46 59
Tested with this example by switching package version between 5.0.4 and latest.
IMHO, it does seem, that the issue could be closed as completed, but let's wait for @flaviendelangle opinion. 馃

@flaviendelangle
Copy link
Member

If it's called less than before, it's nice
I think it's still worth adding a comment on the doc saying that this function is called for every rendered date and that it should not contain any expensive computation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: pickers This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants