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

fix: batch flushSync execution within a microtask #273

Merged
merged 30 commits into from
Nov 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
32997cc
refactor: revert Grid to async rendering
vursen Nov 14, 2024
88c2763
run formatter
vursen Nov 14, 2024
391ea10
update implementation
vursen Nov 15, 2024
cadc3c8
use different approach
vursen Nov 15, 2024
2a25b00
update implementation
vursen Nov 15, 2024
5abefe6
remove unused files
vursen Nov 15, 2024
89ca603
use different approach
vursen Nov 15, 2024
2f6f8d9
update tests to respect async rendering
vursen Nov 15, 2024
7cac073
fix: override grid method in useLayoutEffect
vursen Nov 18, 2024
88dc239
refactor: recalculate column widths also in layout effect
vursen Nov 19, 2024
f735420
use async rendering but defer it to microtask
vursen Nov 19, 2024
e61ea86
extract flushMicrotask as a helper, improve implementation
vursen Nov 20, 2024
fd06a0d
fix grid pro
vursen Nov 20, 2024
666a131
add more tests
vursen Nov 20, 2024
2de1938
run formatter
vursen Nov 20, 2024
2285dd9
Update test/Grid.spec.tsx
vursen Nov 20, 2024
f281097
run formatter
vursen Nov 20, 2024
7a9b3da
run formatter
vursen Nov 20, 2024
b592c47
revert unintentional changes
vursen Nov 20, 2024
bf8c7b9
changes to package.json after building locally
vursen Nov 20, 2024
5a8d15b
Revert "changes to package.json after building locally"
vursen Nov 20, 2024
5450f35
fix import
vursen Nov 20, 2024
6cb6892
remove last blank line in package.json
vursen Nov 20, 2024
d01f67c
explain method overrides in GridPro
vursen Nov 21, 2024
e415bb2
fix another bug
vursen Nov 21, 2024
d961d2a
explain yet another method override
vursen Nov 21, 2024
c6e05c5
fix type in test name
vursen Nov 21, 2024
8405163
improve test coverage
vursen Nov 21, 2024
9199d2f
reduce code repetition in tests
vursen Nov 21, 2024
870c524
Merge branch 'main' into refactor/revert-grid-to-async-rendering
vursen Nov 21, 2024
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
167 changes: 82 additions & 85 deletions packages/react-components-pro/src/GridProEditColumn.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,11 @@
* See https://vaadin.com/commercial-license-and-service-terms for the full
* license.
*/
import React from 'react';
import {
type ForwardedRef,
forwardRef,
type ReactElement,
type ReactNode,
type RefAttributes,
createElement,
} from 'react';
import type { GridBodyRenderer, GridDefaultItem } from '@vaadin/react-components/Grid.js';
import type { GridColumnElement, GridColumnProps } from '@vaadin/react-components/GridColumn.js';
import React, { useLayoutEffect, useRef, useState } from 'react';
import { type ForwardedRef, forwardRef, type ReactElement, type ReactNode, type RefAttributes } from 'react';
import { flushSync } from 'react-dom';
import type { GridDefaultItem } from '@vaadin/react-components/Grid.js';
import type { GridColumnProps } from '@vaadin/react-components/GridColumn.js';
import {
GridProEditColumn as _GridProEditColumn,
type GridProEditColumnElement,
Expand All @@ -27,6 +21,7 @@ import {
import { useModelRenderer } from '@vaadin/react-components/renderers/useModelRenderer.js';
import { useSimpleOrChildrenRenderer } from '@vaadin/react-components/renderers/useSimpleOrChildrenRenderer.js';
import type { OmittedGridColumnHTMLAttributes } from '@vaadin/react-components/GridColumn.js';
import useMergedRefs from '@vaadin/react-components/utils/useMergedRefs.js';

export * from './generated/GridProEditColumn.js';

Expand Down Expand Up @@ -61,99 +56,101 @@ export type GridProEditColumnProps<TItem> = Partial<
renderer?: GridColumnRenderer<TItem>;
}>;

type ReactBodyRenderer<TItem> = GridColumnRenderer<TItem> & {
__wrapperRenderer?: ReactBodyRenderer<TItem>;
type GridProEditColumnElementInternals<TItem> = {
_clearCellContent(cell: HTMLElement & { [SKIP_CLEARING_CELL_CONTENT]?: boolean }): void;
_renderEditor(cell: HTMLElement & { [SKIP_CLEARING_CELL_CONTENT]?: boolean }, model: { item: TItem }): void;
_removeEditor(cell: HTMLElement & { [SKIP_CLEARING_CELL_CONTENT]?: boolean }, model: { item: TItem }): void;
};

type EditColumnRendererRoot = HTMLElement & { __editColumnRenderer?: GridBodyRenderer };

type ClearFunction = (arg0: HTMLElement & { _content: EditColumnRendererRoot }) => void;

/**
* Wraps a React renderer function to render empty when requested
*
* @returns
*/
function editColumnReactRenderer<TItem>(reactBodyRenderer?: ReactBodyRenderer<TItem> | null) {
if (!reactBodyRenderer) {
return undefined;
}

reactBodyRenderer.__wrapperRenderer ||= function GridProEditColumnRenderer(props) {
// If the model has __renderEmpty set, return null, otherwise call the original renderer
return '__renderEmpty' in props.model ? null : createElement(reactBodyRenderer, props);
};

return reactBodyRenderer.__wrapperRenderer;
}

/**
* Wraps a Grid body renderer function to make it request empty render before
* the GridPro edit column clears cell content.
*/
function editColumnRenderer(bodyRenderer?: (GridBodyRenderer & { __wrapperRenderer?: GridBodyRenderer }) | null) {
if (!bodyRenderer) {
return undefined;
}

bodyRenderer.__wrapperRenderer ||= (
root: EditColumnRendererRoot,
column: GridColumnElement & {
__originalClearCellContent?: ClearFunction;
_clearCellContent?: ClearFunction;
},
model,
) => {
// Patch the column's _clearCellContent function which is called internally by grid-pro
// when switching from edit mode to view mode and vice versa
if (!column.__originalClearCellContent) {
column.__originalClearCellContent = column._clearCellContent;

column._clearCellContent = (cell) => {
const cellRoot = cell._content;
// Call the original renderer with __renderEmpty set to true to clear the content it manages
cellRoot.__editColumnRenderer?.(cellRoot, column, Object.assign({}, model, { __renderEmpty: true }));
// Call the original clearCellContent function to manually clear the cell content
column.__originalClearCellContent?.(cell);
};
}

// Update the cell content's renderer reference so that the correct one is used
// to render empty when the cell is cleared
root.__editColumnRenderer = bodyRenderer;

// Call the original renderer
bodyRenderer(root, column, model);
};

return bodyRenderer.__wrapperRenderer;
}
const SKIP_CLEARING_CELL_CONTENT = Symbol();

function GridProEditColumn<TItem = GridDefaultItem>(
{ children, footer, header, ...props }: GridProEditColumnProps<TItem>,
ref: ForwardedRef<GridProEditColumnElement<TItem>>,
): ReactElement | null {
const [editModePortals, editModeRenderer] = useModelRenderer(editColumnReactRenderer(props.editModeRenderer), {
renderSync: true,
const [editedItem, setEditedItem] = useState<TItem | null>(null);

const [editModePortals, editModeRenderer] = useModelRenderer(props.editModeRenderer, {
// The web component implementation currently requires the editor to be rendered synchronously.
renderMode: 'sync',
shouldRenderPortal: (_root, _column, model) => editedItem === model.item,
});
const [headerPortals, headerRenderer] = useSimpleOrChildrenRenderer(props.headerRenderer, header, {
renderSync: true,
renderMode: 'microtask',
});
const [footerPortals, footerRenderer] = useSimpleOrChildrenRenderer(props.footerRenderer, footer, {
renderSync: true,
renderMode: 'microtask',
});
const [bodyPortals, bodyRenderer] = useModelRenderer(editColumnReactRenderer(props.renderer ?? children), {
renderSync: true,
const [bodyPortals, bodyRenderer] = useModelRenderer(props.renderer ?? children, {
renderMode: 'microtask',
shouldRenderPortal: (_root, _column, model) => editedItem !== model.item,
});

const innerRef = useRef<GridProEditColumnElement<TItem> & GridProEditColumnElementInternals<TItem>>(null);
const finalRef = useMergedRefs(innerRef, ref);

useLayoutEffect(() => {
innerRef.current!._clearCellContent = function (cell) {
// Clearing cell content in _renderEditor and _removeEditor is decided
// based on whether the content was rendered by a React renderer or not.
if (!cell[SKIP_CLEARING_CELL_CONTENT]) {
Object.getPrototypeOf(this)._clearCellContent.call(this, cell);
}
};
}, []);

useLayoutEffect(() => {
innerRef.current!._renderEditor = function (cell, model) {
// Manually clear the cell content only if it was rendered by the default grid renderer.
// For content rendered by a React renderer, clearing is handled by removing the portal.
if (!bodyRenderer) {
this._clearCellContent(cell);
}
tomivirkki marked this conversation as resolved.
Show resolved Hide resolved

// Ensure the corresponding bodyRenderer portal is removed and the editModeRenderer portal
// is added instead.
flushSync(() => {
setEditedItem(model.item);
});

cell[SKIP_CLEARING_CELL_CONTENT] = true;
Object.getPrototypeOf(this)._renderEditor.call(this, cell, model);
cell[SKIP_CLEARING_CELL_CONTENT] = false;
};
}, [bodyRenderer]);

useLayoutEffect(() => {
innerRef.current!._removeEditor = function (cell, model) {
// Manually clear the cell content only if it was rendered by the default grid renderer.
// For content rendered by a React renderer, clearing is handled by removing the portal.
if (!editModeRenderer) {
this._clearCellContent(cell);
}

// Ensure the editModeRenderer portal is removed and the corresponding bodyRenderer portal
// is added again. Please note the bodyRenderer portal will be added synchronously even though
// the renderer has renderMode set to microtask. It's because the portal already has content
// from the previous render cycle and we just show it again.
flushSync(() => {
setEditedItem((editedItem) => {
return editedItem === model.item ? null : editedItem;
});
});

cell[SKIP_CLEARING_CELL_CONTENT] = true;
Object.getPrototypeOf(this)._removeEditor.call(this, cell, model);
cell[SKIP_CLEARING_CELL_CONTENT] = false;
};
}, [editModeRenderer]);

return (
<_GridProEditColumn<TItem>
{...props}
editModeRenderer={editColumnRenderer(editModeRenderer)}
editModeRenderer={editModeRenderer}
footerRenderer={footerRenderer}
headerRenderer={headerRenderer}
ref={ref}
renderer={editColumnRenderer(bodyRenderer)}
ref={finalRef}
renderer={bodyRenderer}
>
{editModePortals}
{headerPortals}
Expand Down
4 changes: 4 additions & 0 deletions packages/react-components/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,10 @@
"./utils/createComponentWithOrderedProps.d.ts.map": "./utils/createComponentWithOrderedProps.d.ts.map",
"./utils/createComponentWithOrderedProps.js": "./utils/createComponentWithOrderedProps.js",
"./utils/createComponentWithOrderedProps.js.map": "./utils/createComponentWithOrderedProps.js.map",
"./utils/flushMicrotask.d.ts": "./utils/flushMicrotask.d.ts",
"./utils/flushMicrotask.d.ts.map": "./utils/flushMicrotask.d.ts.map",
"./utils/flushMicrotask.js": "./utils/flushMicrotask.js",
"./utils/flushMicrotask.js.map": "./utils/flushMicrotask.js.map",
"./utils/mapItemsWithComponents.d.ts": "./utils/mapItemsWithComponents.d.ts",
"./utils/mapItemsWithComponents.d.ts.map": "./utils/mapItemsWithComponents.d.ts.map",
"./utils/mapItemsWithComponents.js": "./utils/mapItemsWithComponents.js",
Expand Down
41 changes: 26 additions & 15 deletions packages/react-components/src/Grid.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
import { type ComponentType, type ForwardedRef, forwardRef, type ReactElement, type RefAttributes } from 'react';
import {
type ComponentType,
type ForwardedRef,
forwardRef,
type ReactElement,
type RefAttributes,
useLayoutEffect,
useRef,
} from 'react';
import {
Grid as _Grid,
type GridDefaultItem,
Expand All @@ -7,6 +15,7 @@ import {
} from './generated/Grid.js';
import type { GridRowDetailsReactRendererProps } from './renderers/grid.js';
import { useModelRenderer } from './renderers/useModelRenderer.js';
import useMergedRefs from './utils/useMergedRefs.js';

export * from './generated/Grid.js';

Expand All @@ -19,10 +28,24 @@ function Grid<TItem = GridDefaultItem>(
props: GridProps<TItem>,
ref: ForwardedRef<GridElement<TItem>>,
): ReactElement | null {
const [portals, rowDetailsRenderer] = useModelRenderer(props.rowDetailsRenderer, { renderSync: true });
const [portals, rowDetailsRenderer] = useModelRenderer(props.rowDetailsRenderer, {
renderMode: 'microtask',
});

const innerRef = useRef<GridElement>(null);
const finalRef = useMergedRefs(innerRef, ref);

useLayoutEffect(() => {
innerRef.current!.recalculateColumnWidths = function (...args) {
tomivirkki marked this conversation as resolved.
Show resolved Hide resolved
// Wait for column content to finish rendering before recalculating widths.
queueMicrotask(() => {
Object.getPrototypeOf(this).recalculateColumnWidths.call(this, ...args);
});
};
}, []);

return (
<_Grid<TItem> {...props} ref={ref} rowDetailsRenderer={rowDetailsRenderer}>
<_Grid<TItem> {...props} ref={finalRef} rowDetailsRenderer={rowDetailsRenderer}>
{props.children}
{portals}
</_Grid>
Expand All @@ -34,15 +57,3 @@ const ForwardedGrid = forwardRef(Grid) as <TItem = GridDefaultItem>(
) => ReactElement | null;

export { ForwardedGrid as Grid };

customElements.whenDefined('vaadin-grid').then(() => {
const gridProto = customElements.get('vaadin-grid')?.prototype;
const originalRecalculateColumnWidths = gridProto?._recalculateColumnWidths;
gridProto._recalculateColumnWidths = function (...args: any[]) {
// Multiple synchronous calls to the renderers using flushSync cause
// some of the renderers to be called asynchronously (see useRenderer.ts).
// To make sure all the column cell content is rendered before recalculating
// the column widths, we need to make _recalculateColumnWidths asynchronous.
queueMicrotask(() => originalRecalculateColumnWidths.call(this, ...args));
};
});
6 changes: 3 additions & 3 deletions packages/react-components/src/GridColumn.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ function GridColumn<TItem = GridDefaultItem>(
ref: ForwardedRef<GridColumnElement<TItem>>,
): ReactElement | null {
const [headerPortals, headerRenderer] = useSimpleOrChildrenRenderer(props.headerRenderer, header, {
renderSync: true,
renderMode: 'microtask',
});
const [footerPortals, footerRenderer] = useSimpleOrChildrenRenderer(props.footerRenderer, footer, {
renderSync: true,
renderMode: 'microtask',
});
const [bodyPortals, bodyRenderer] = useModelRenderer(props.renderer ?? children, {
renderSync: true,
renderMode: 'microtask',
});

return (
Expand Down
4 changes: 2 additions & 2 deletions packages/react-components/src/GridColumnGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ function GridColumnGroup(
ref: ForwardedRef<GridColumnGroupElement>,
): ReactElement | null {
const [headerPortals, headerRenderer] = useSimpleOrChildrenRenderer(props.headerRenderer, header, {
renderSync: true,
renderMode: 'microtask',
});
const [footerPortals, footerRenderer] = useSimpleOrChildrenRenderer(props.footerRenderer, footer, {
renderSync: true,
renderMode: 'microtask',
});

return (
Expand Down
4 changes: 2 additions & 2 deletions packages/react-components/src/GridFilterColumn.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ function GridFilterColumn<TItem = GridDefaultItem>(
ref: ForwardedRef<GridFilterColumnElement<TItem>>,
): ReactElement | null {
const [footerPortals, footerRenderer] = useSimpleOrChildrenRenderer(props.footerRenderer, footer, {
renderSync: true,
renderMode: 'microtask',
});
const [bodyPortals, bodyRenderer] = useModelRenderer(props.renderer ?? props.children, {
renderSync: true,
renderMode: 'microtask',
});

return (
Expand Down
6 changes: 3 additions & 3 deletions packages/react-components/src/GridSelectionColumn.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,13 +50,13 @@ function GridSelectionColumn<TItem = GridDefaultItem>(
ref: ForwardedRef<GridSelectionColumnElement<TItem>>,
): ReactElement | null {
const [headerPortals, headerRenderer] = useSimpleOrChildrenRenderer(props.headerRenderer, header, {
renderSync: true,
renderMode: 'microtask',
});
const [footerPortals, footerRenderer] = useSimpleOrChildrenRenderer(props.footerRenderer, footer, {
renderSync: true,
renderMode: 'microtask',
});
const [bodyPortals, bodyRenderer] = useModelRenderer(props.renderer ?? props.children, {
renderSync: true,
renderMode: 'microtask',
});

return (
Expand Down
4 changes: 2 additions & 2 deletions packages/react-components/src/GridSortColumn.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@ function GridSortColumn<TItem = GridDefaultItem>(
ref: ForwardedRef<GridSortColumnElement<TItem>>,
): ReactElement | null {
const [footerPortals, footerRenderer] = useSimpleOrChildrenRenderer(props.footerRenderer, footer, {
renderSync: true,
renderMode: 'microtask',
});
const [bodyPortals, bodyRenderer] = useModelRenderer(props.renderer ?? props.children, {
renderSync: true,
renderMode: 'microtask',
});

return (
Expand Down
4 changes: 2 additions & 2 deletions packages/react-components/src/GridTreeColumn.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ function GridTreeColumn<TItem = GridDefaultItem>(
ref: ForwardedRef<GridTreeColumnElement<TItem>>,
): ReactElement | null {
const [headerPortals, headerRenderer] = useSimpleOrChildrenRenderer(props.headerRenderer, header, {
renderSync: true,
renderMode: 'microtask',
});
const [footerPortals, footerRenderer] = useSimpleOrChildrenRenderer(props.footerRenderer, footer, {
renderSync: true,
renderMode: 'microtask',
});

return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ export function convertModelRendererArgs<I, M extends Model<I>, O extends HTMLEl

export function useModelRenderer<I, M extends Model<I>, O extends HTMLElement>(
reactRenderer?: ComponentType<ReactModelRendererProps<I, M, O>> | null,
config?: RendererConfig,
config?: RendererConfig<WebComponentModelRenderer<I, M, O>>,
): UseRendererResult<WebComponentModelRenderer<I, M, O>> {
return useRenderer(reactRenderer, convertModelRendererArgs, config);
}
Loading
Loading