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

[core][Slider] Ref immutable consistency #227

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Mar 23, 2024

Make it trigger as an immutable ref, for consistency with the rest of the codebase.

To be fair, I don't know if this PR (Option A) is the right long-term decision. We have two other options:

Option B

-const sliderRef = React.useRef<HTMLSpanElement>(null);
+const sliderRef = React.useRef<HTMLSpanElement>() as React.RefObject<HTMLSpanElement>;

Option C

-const sliderRef = React.useRef<HTMLSpanElement>(null);
+const sliderRef = React.useRef<HTMLSpanElement>();

I started a bit this with @romgrk in mui/mui-x#11847 (review).

  • Option A leads to more bundle size, but is consistent with how React sets null when unmounting. It also prevents reassignment mistakes. What's documented in https://react.dev/reference/react/useRef.
  • Option B reduces bundle size, avoids reassignment mistakes, and treats null/undefined as equivalent
  • Option C doesn't have the type of safety benefit.

In any case, for now, I'm going with the consistency path, defaulting to what the rest of the codebase is doing. It makes it easier to learn and update things at scale. It feels to me that:

  • We shouldn't care about null/undefined accuracy, we can treat them as equivalent
  • We should care about type reassignment
  • We should care about bundle size, maintainers should pay the cost, not developers or end-users.

So I land on going with mui/mui-x#11847 (comment) for the none refs because who cares about the null/undefiend difference. Going with Option A for DOM refs to things simple for the rest. If this is OK, I can open an issue. It probably doesn't matter much what we pick, as long as it's consistent.

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes typescript labels Mar 23, 2024
@michaldudak michaldudak changed the title [slider][core] Ref immutable consistency [core][Slider] Ref immutable consistency Apr 12, 2024
@michaldudak michaldudak merged commit 2b2836f into mui:master Apr 12, 2024
17 of 18 checks passed
@oliviertassinari oliviertassinari deleted the immutable-ref branch May 3, 2024 23:06
@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 3, 2024

React 19 requires useRef() to have an argument. I got lucky:

SCR-20240504-byad

https://react.dev/blog/2024/04/25/react-19-upgrade-guide#useref-requires-argument

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants