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] Rely on immutable ref when possible #11847

Merged
merged 4 commits into from
Jan 29, 2024

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari added core Infrastructure work going on behind the scenes typescript labels Jan 28, 2024
@mui-bot
Copy link

mui-bot commented Jan 28, 2024

Deploy preview: https://deploy-preview-11847--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 8e5c560

Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

I'm not sure about this change, it's more verbose and doesn't seem to change anything in practice. It's also not symmetric for the refs that have an agreed upon empty value (useRef<Element[]>([])).

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jan 28, 2024

@romgrk Do you mean we don't need these changes in packages/grid/x-data-grid-pro/src/hooks/features/columnResize/useGridColumnResize.tsx as it's already a mutable ref without them? Yeah, we could do this, say that we treat null or undefined equality so we can be more concise. Now, the rest of the codebase follows this approach, so I think it should be for a different PR, one where we update the rest of the codebase.

I do think it should be a immutable ref in the demos.

@romgrk
Copy link
Contributor

romgrk commented Jan 28, 2024

Yes I'd prefer we keep useGridResize as it is for now. It's ok to change the demos.

@oliviertassinari oliviertassinari merged commit a9b7fc7 into mui:next Jan 29, 2024
17 checks passed
@oliviertassinari oliviertassinari deleted the non-mutable-ref branch January 29, 2024 13:57
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jan 29, 2024

@romgrk Going forward, it would make sense to me to change the default value for mutable refs or non React ref, to use undefined, over null. How would this diff sound?

diff --git a/packages/grid/x-data-grid-pro/src/hooks/features/columnReorder/useGridColumnReorder.tsx b/packages/grid/x-data-grid-pro/src/hooks/features/columnReorder/useGridColumnReorder.tsx
index 68c773980..565612ab5 100644
--- a/packages/grid/x-data-grid-pro/src/hooks/features/columnReorder/useGridColumnReorder.tsx
+++ b/packages/grid/x-data-grid-pro/src/hooks/features/columnReorder/useGridColumnReorder.tsx
@@ -65,12 +65,12 @@ export const useGridColumnReorder = (
 ): void => {
   const logger = useGridLogger(apiRef, 'useGridColumnReorder');

-  const dragColNode = React.useRef<HTMLElement | null>(null);
+  const dragColNode = React.useRef<HTMLElement>();
   const cursorPosition = React.useRef<CursorCoordinates>({
     x: 0,
     y: 0,
   });
-  const originColumnIndex = React.useRef<number | null>(null);
+  const originColumnIndex = React.useRef<number>();
   const forbiddenIndexes = React.useRef<{ [key: number]: boolean }>({});
   const removeDnDStylesTimeout = React.useRef<any>();
   const ownerState = { classes: props.classes };
@@ -315,13 +315,13 @@ export const useGridColumnReorder = (
       event.stopPropagation();

       clearTimeout(removeDnDStylesTimeout.current);
-      dragColNode.current = null;
+      dragColNode.current = undefined;

       // Check if the column was dropped outside the grid.
       if (event.dataTransfer.dropEffect === 'none' && !props.keepColumnPositionIfDraggedOutside) {
         // Accessing params.field may contain the wrong field as header elements are reused
         apiRef.current.setColumnIndex(dragColField, originColumnIndex.current!);
-        originColumnIndex.current = null;
+        originColumnIndex.current = undefined;
       } else {
         // Emit the columnOrderChange event only once when the reordering stops.
         const columnOrderChangeParams: GridColumnOrderChangeParams = {
diff --git a/packages/grid/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx b/packages/grid/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx
index 6e2b3ca69..52359b098 100644
--- a/packages/grid/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx
+++ b/packages/grid/x-data-grid-pro/src/hooks/features/rowReorder/useGridRowReorder.tsx
@@ -57,7 +57,7 @@ export const useGridRowReorder = (
   const logger = useGridLogger(apiRef, 'useGridRowReorder');
   const sortModel = useGridSelector(apiRef, gridSortModelSelector);
   const treeDepth = useGridSelector(apiRef, gridRowMaximumTreeDepthSelector);
-  const dragRowNode = React.useRef<HTMLElement | null>(null);
+  const dragRowNode = React.useRef<HTMLElement>();
   const originRowIndex = React.useRef<number | null>(null);
   const removeDnDStylesTimeout = React.useRef<any>();
   const ownerState = { classes: props.classes };
@@ -167,7 +167,7 @@ export const useGridRowReorder = (
       event.stopPropagation();

       clearTimeout(removeDnDStylesTimeout.current);
-      dragRowNode.current = null;
+      dragRowNode.current = undefined;
       previousReorderState.dragDirection = null;

       // Check if the row was dropped outside the grid.
diff --git a/packages/grid/x-data-grid/src/components/menu/GridMenu.tsx b/packages/grid/x-data-grid/src/components/menu/GridMenu.tsx
index cc00c12da..39a1f3562 100644
--- a/packages/grid/x-data-grid/src/components/menu/GridMenu.tsx
+++ b/packages/grid/x-data-grid/src/components/menu/GridMenu.tsx
@@ -74,14 +74,14 @@ function GridMenu(props: GridMenuProps) {
   const rootProps = useGridRootProps();
   const classes = useUtilityClasses(rootProps);

-  const savedFocusRef = React.useRef<HTMLElement | null>(null);
+  const savedFocusRef = React.useRef<HTMLElement>();
   useEnhancedEffect(() => {
     if (open) {
       savedFocusRef.current =
-        document.activeElement instanceof HTMLElement ? document.activeElement : null;
+        document.activeElement instanceof HTMLElement ? document.activeElement : undefined;
     } else {
       savedFocusRef.current?.focus?.();
-      savedFocusRef.current = null;
+      savedFocusRef.current = undefined;
     }
   }, [open]);

diff --git a/packages/x-date-pickers/src/MultiSectionDigitalClock/MultiSectionDigitalClockSection.tsx b/packages/x-date-pickers/src/MultiSectionDigitalCloc
k/MultiSectionDigitalClockSection.tsx
index ced796ed2..202c0919b 100644
--- a/packages/x-date-pickers/src/MultiSectionDigitalClock/MultiSectionDigitalClockSection.tsx
+++ b/packages/x-date-pickers/src/MultiSectionDigitalClock/MultiSectionDigitalClockSection.tsx
@@ -126,7 +126,7 @@ export const MultiSectionDigitalClockSection = React.forwardRef(
   ) {
     const containerRef = React.useRef<HTMLUListElement>(null);
     const handleRef = useForkRef(ref, containerRef);
-    const previousActive = React.useRef<HTMLElement | null>(null);
+    const previousActive = React.useRef<HTMLElement>();

     const props = useThemeProps({
       props: inProps,

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.

3 participants