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

[DataGrid] Refactor rendering, remove rafUpdate #532

Merged
merged 19 commits into from
Nov 4, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions packages/grid/_modules_/grid/GridComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -81,17 +81,17 @@ export const GridComponent = React.forwardRef<HTMLDivElement, GridComponentProps

const customComponents = useComponents(props.components, apiRef, rootContainerRef);

logger.info(
`Rendering, page: ${renderCtx?.page}, col: ${renderCtx?.firstColIdx}-${renderCtx?.lastColIdx}, row: ${renderCtx?.firstRowIdx}-${renderCtx?.lastRowIdx}`,
renderCtx,
);

// TODO move that to renderCtx
const getTotalHeight = React.useCallback(
(size) => getCurryTotalHeight(gridState.options, gridState.containerSizes, footerRef)(size),
[gridState.options, gridState.containerSizes],
);

logger.info(
`Rendering, page: ${renderCtx?.page}, col: ${renderCtx?.firstColIdx}-${renderCtx?.lastColIdx}, row: ${renderCtx?.firstRowIdx}-${renderCtx?.lastRowIdx}`,
apiRef.current.state,
);

return (
<AutoSizer onResize={onResize}>
{(size: any) => (
Expand Down
13 changes: 7 additions & 6 deletions packages/grid/_modules_/grid/components/AutoSizer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -96,13 +96,13 @@ export const AutoSizer = React.forwardRef<HTMLDivElement, AutoSizerProps>(functi
(!disableHeight && state.height !== newHeight) ||
(!disableWidth && state.width !== newWidth)
) {
setState({
height: height - paddingTop - paddingBottom,
width: width - paddingLeft - paddingRight,
});
setState(() => ({
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
height: newHeight,
width: newWidth,
}));

if (onResize) {
onResize({ height, width });
onResize({ height: newHeight, width: newWidth });
}
}
}
Expand All @@ -119,10 +119,12 @@ export const AutoSizer = React.forwardRef<HTMLDivElement, AutoSizerProps>(functi

const detectElementResize = createDetectElementResize(nonce, win);
detectElementResize.addResizeListener(parentElement.current, handleResize);
window.addEventListener('resize', handleResize);
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved
handleResize();

return () => {
detectElementResize.removeResizeListener(parentElement.current, handleResize);
window.removeEventListener('resize', handleResize);
};
}, [nonce, handleResize]);

Expand All @@ -143,7 +145,6 @@ export const AutoSizer = React.forwardRef<HTMLDivElement, AutoSizerProps>(functi
}

const handleRef = useForkRef(rootRef, ref);

return (
<div
ref={handleRef}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import * as React from 'react';
import { ApiRef } from '../../../models/api/apiRef';
import { GridApi } from '../../../models/api/gridApi';
import { useLogger } from '../../utils/useLogger';
import { getInitialState } from './gridState';

export const useGridApi = (apiRef: ApiRef): GridApi => {
const logger = useLogger('useGridApi');
const [, forceUpdate] = React.useState();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const [, forceUpdate] = React.useState();
const [, forceUpdate] = React.useReducer((acc) => acc + 1, 0);

Copy link
Member Author

Choose a reason for hiding this comment

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

Why? I prefer to useState so I can pass the state I need...

Copy link
Member

@oliviertassinari oliviertassinari Nov 3, 2020

Choose a reason for hiding this comment

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

The current usage force you to call forceUpdate with an argument, and it has to have a unique reference, like a new object or a state setting that returns a unique reference. In the current logic, it's called with a function. It could be simpler if it didn't need to accept an argument, and maybe less error prone?

Copy link
Member Author

Choose a reason for hiding this comment

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

it does, you just need to pass a function

Copy link
Member

Choose a reason for hiding this comment

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

Why? I prefer to useState so I can pass the state I need...

The name forceUpdate is misleading. You have to pass the correct parameter to schedule work.

Though generally forcing a re-render is rarely needed. Whenever you forceUpdate why can't you move whatever changed into state?

Copy link
Member

@oliviertassinari oliviertassinari Nov 3, 2020

Choose a reason for hiding this comment

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

There is one case that took me some time to understand, I think that by doing

apiRef.current.forceUpdate(() => apiRef.current.state),

We can skip updates when the state hasn't changed, which can be an interesting feature. I was confused by the name, we definitely need to find a different one. IMHO forceUpdate === no arguments. What about setState?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is not a setState, it's just to force the ui to rerender

Copy link
Member

Choose a reason for hiding this comment

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

@dtassone Are you saying that all we need is a new reference? If its the case, I would rather do:

-apiRef.current.forceUpdate(() => apiRef.current.state),
+apiRef.current.forceUpdate({}),

to remove the dependency another part of the logic.

Copy link
Member

Choose a reason for hiding this comment

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

this is not a setState, it's just to force the ui to rerender

But it is? Assuming you refer to setState returned from the useState hook then I don't see any difference. Both setState and forceUpdate have to know what the current state is to actually force a re-rerender (which isn't clear why you need this).

if (!apiRef.current.isInitialised && !apiRef.current.state) {
logger.info('Initialising state.');
apiRef.current.state = getInitialState();
apiRef.current.forceUpdate = forceUpdate;
apiRef.current.getState = <State>(stateId?: string) =>
(stateId ? apiRef.current.state[stateId] : apiRef.current.state) as State;
}
Expand Down
11 changes: 5 additions & 6 deletions packages/grid/_modules_/grid/hooks/features/core/useGridState.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,21 @@
import * as React from 'react';
import { ApiRef } from '../../../models/api/apiRef';
import { useRafUpdate } from '../../utils/useRafUpdate';
import { GridState } from './gridState';
import { useGridApi } from './useGridApi';

export const useGridState = (
apiRef: ApiRef,
): [GridState, (stateUpdaterFn: (oldState: GridState) => GridState) => void, () => void] => {
const api = useGridApi(apiRef);
const [, forceUpdate] = React.useState();
const [rafUpdate] = useRafUpdate(apiRef, () => forceUpdate((p: any) => !p));

const forceUpdate = React.useCallback(
() => apiRef.current.forceUpdate(() => apiRef.current.state),
dtassone marked this conversation as resolved.
Show resolved Hide resolved
[apiRef],
);
const setGridState = React.useCallback(
(stateUpdaterFn: (oldState: GridState) => GridState) => {
api.state = stateUpdaterFn(api.state);
},
[api],
);

return [api.state, setGridState, rafUpdate];
return [api.state, setGridState, forceUpdate];
};
23 changes: 18 additions & 5 deletions packages/grid/_modules_/grid/hooks/features/rows/useRows.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,25 @@ export function convertRowsPropToState({

export const useRows = (rows: RowsProp, apiRef: ApiRef): void => {
const logger = useLogger('useRows');
const [gridState, setGridState, forceUpdate] = useGridState(apiRef);
const [gridState, setGridState, updateComponent] = useGridState(apiRef);
const updateTimeout = React.useRef<any>();

const forceUpdate = React.useCallback(() => {
if (updateTimeout!.current == null) {
updateTimeout!.current = setTimeout(() => {
logger.debug(`Updating component`);
updateTimeout.current = null;
return updateComponent();
}, 100);
}
}, [logger, updateComponent]);

const internalRowsState = React.useRef<InternalRowsState>(gridState.rows);

React.useEffect(() => {
return () => clearTimeout(updateTimeout!.current);
}, []);

React.useEffect(() => {
setGridState((state) => {
internalRowsState.current = convertRowsPropToState({
Expand Down Expand Up @@ -85,7 +100,7 @@ export const useRows = (rows: RowsProp, apiRef: ApiRef): void => {

const updateRowModels = React.useCallback(
(updates: Partial<RowModel>[]) => {
logger.debug(`updating ${updates.length} row models`);
logger.debug(`updating row models`);
const addedRows: RowModel[] = [];

updates.forEach((partialRow) => {
Expand Down Expand Up @@ -121,8 +136,6 @@ export const useRows = (rows: RowsProp, apiRef: ApiRef): void => {

const updateRowData = React.useCallback(
(updates: RowData[]) => {
logger.debug(`updating rows data`);

// we removes duplicate updates. A server can batch updates, and send several updates for the same row in one fn call.
const uniqUpdates = updates.reduce((uniq, update) => {
if (update.id == null) {
Expand All @@ -148,7 +161,7 @@ export const useRows = (rows: RowsProp, apiRef: ApiRef): void => {
});
return updateRowModels(rowModelUpdates);
},
[updateRowModels, logger, getRowFromId],
[updateRowModels, getRowFromId],
);

const getRowModels = React.useCallback(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ export const useVirtualRows = (
const logger = useLogger('useVirtualRows');

const updateViewportRef = React.useRef<(...args: any[]) => void>();
const [, forceUpdate] = React.useState();
const [gridState, setGridState, rafUpdate] = useGridState(apiRef);
const [gridState, setGridState, forceUpdate] = useGridState(apiRef);
const options = useGridSelector(apiRef, optionsSelector);
const paginationState = useGridSelector<PaginationState>(apiRef, paginationSelector);
const totalRowCount = useGridSelector<number>(apiRef, rowCountSelector);
Expand All @@ -51,6 +50,7 @@ export const useVirtualRows = (
if (!isEqual(oldState.rendering, currentRenderingState)) {
oldState.rendering = currentRenderingState;
stateChanged = true;
return { ...oldState };
dtassone marked this conversation as resolved.
Show resolved Hide resolved
}
return oldState;
});
Expand Down Expand Up @@ -104,16 +104,10 @@ export const useVirtualRows = (
renderedSizes: apiRef.current.state.containerSizes,
});
if (hasChanged) {
if (apiRef.current.state.isScrolling) {
logger.debug('reRender: Raf rendering');
rafUpdate();
} else {
logger.debug('reRender: Force rendering');
// we force this update, the func makes react force run this state update and rerender
forceUpdate((p) => !p);
}
logger.debug('reRender: trigger rendering');
forceUpdate();
}
}, [apiRef, getRenderingState, logger, rafUpdate, setRenderingState]);
}, [apiRef, getRenderingState, logger, forceUpdate, setRenderingState]);

const updateViewport = React.useCallback(
(forceReRender = false) => {
Expand Down Expand Up @@ -272,8 +266,7 @@ export const useVirtualRows = (
scrollingTimeout.current = setTimeout(() => {
scrollingTimeout.current = null;
apiRef.current.state.isScrolling = false;
// We let react decide to run this update.
forceUpdate(true);
forceUpdate();
}, 300);

if (updateViewportRef.current) {
Expand Down
7 changes: 6 additions & 1 deletion packages/grid/_modules_/grid/hooks/root/useContainerProps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,18 @@ export const useContainerProps = (windowRef: React.RefObject<HTMLDivElement>, ap

const updateContainerState = React.useCallback(
(containerState: ContainerProps | null) => {
let updated = false;
setGridState((state) => {
if (!isEqual(state.containerSizes, containerState)) {
state.containerSizes = containerState;
forceUpdate();
updated = true;
return { ...state };
dtassone marked this conversation as resolved.
Show resolved Hide resolved
}
return state;
});
if (updated) {
forceUpdate();
}
},
[forceUpdate, setGridState],
);
Expand Down
1 change: 0 additions & 1 deletion packages/grid/_modules_/grid/hooks/utils/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,2 @@
export * from './useLogger';
export * from './useRafUpdate';
export * from './useScrollFn';
43 changes: 0 additions & 43 deletions packages/grid/_modules_/grid/hooks/utils/useRafUpdate.ts

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export function useResizeContainer(apiRef): (size: ElementSize) => void {
},
[gridLogger, apiRef],
);
const debouncedOnResize = React.useMemo(() => debounce(onResize, 100), [onResize]) as any;
const debouncedOnResize = React.useMemo(() => debounce(onResize, 10), [onResize]) as any;
oliviertassinari marked this conversation as resolved.
Show resolved Hide resolved

React.useEffect(() => {
return () => {
Expand Down
7 changes: 2 additions & 5 deletions packages/grid/_modules_/grid/hooks/utils/useScrollFn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,12 @@ import { debounce } from '@material-ui/core/utils';
import * as React from 'react';
import { ScrollFn, ScrollParams } from '../../models/params/scrollParams';
import { useLogger } from './useLogger';
import { useRafUpdate } from './useRafUpdate';

export function useScrollFn(
apiRef: any,
renderingZoneElementRef: React.RefObject<HTMLDivElement>,
columnHeadersElementRef: React.RefObject<HTMLDivElement>,
): [ScrollFn, ScrollFn] {
): [ScrollFn] {
const logger = useLogger('useScrollFn');
const previousValue = React.useRef<ScrollParams>();

Expand Down Expand Up @@ -40,13 +39,11 @@ export function useScrollFn(
[renderingZoneElementRef, logger, columnHeadersElementRef, debouncedResetPointerEvents],
);

const [runScroll] = useRafUpdate(apiRef, scrollTo);

React.useEffect(() => {
return () => {
debouncedResetPointerEvents.clear();
};
}, [renderingZoneElementRef, debouncedResetPointerEvents]);

return [runScroll, scrollTo];
return [scrollTo];
}
14 changes: 0 additions & 14 deletions packages/grid/_modules_/grid/hooks/utils/useStateRef.ts

This file was deleted.

5 changes: 5 additions & 0 deletions packages/grid/_modules_/grid/models/api/stateApi.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as React from 'react';
import { GridState } from '../../hooks/features/core/gridState';

export interface StateApi {
Expand All @@ -9,4 +10,8 @@ export interface StateApi {
* allows to get the whole state of the grid if stateId is null or to get a part of the state if stateId has a value.
*/
getState: <T>(stateId?: string) => T;
/**
* allows to force the grid to rerender after a state update
dtassone marked this conversation as resolved.
Show resolved Hide resolved
*/
forceUpdate: React.Dispatch<any>;
}
Loading