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] Move away for the event system to trigger pipe processings #4378

Merged
merged 4 commits into from
Apr 11, 2022

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Apr 6, 2022

Extracted from #4208

This PR only impacts the pipe processing which are triggered when GridEvents.columnsChange is fired. Pipe processing lilke columnMenu or scrollToIndexes are not impacted.

Current behavior for processors

  1. Plugin A registers a processor
  2. Plugin B run the processors on mount
  3. Plugin B listens to GridEvents.pipeProcessorRegister and re-apply the processors whenever this event is fired on the right group
  4. Plugin A has an update on one of the deps of its processor, it triggers a re-registration and therefore a re-application.

A problem appears when we want to re-apply the processors but none of the deps of the processor have changed.

For instance in the row grouping, we want to re-apply hydrateColumns when the columns in a way which changes the sanitized rowGroupingModel.
If we just use gridRowGroupingSanitizedModelSelector in the render of useGridRowGroupingProcessors to update the deps of the processor, then we would have two problems:

  1. Any change to the columns would re-apply hydrateColumns, creating an infinite loop
  2. The processor re-registration being in an effect, the grid would re-render with the new columns but the old grouping column and then a 2nd time with the new columns and the new grouping column.

The current hack to listen to GridEvents.columnsChange and to fire manually hydrateColumns by calling apiRef.current.updateColumns([]).
This hacky solutions works and re-apply hydrateColumns synchronously when the columns are updated in a way that changes the sanitized rowGroupingModel.

Why do we need to fix it ?

For the aggregation, I need to do something similar.
When the columns changes, I need to re-apply:

  • hydrateColumns to wrap / unwrap the aggregated columns (the same hack can be used)
  • hydrateRows (a new pipe processing that adds rows and is used by the aggregation to create the footers)

The same hack can't be used for hydrateRows.
If I call apiRef.current.updateRows([]), the update of the state is throttled which is problematic here.
I could create a new apiRef.current.unstable_syncUpdateRows, but I think it is worth cleaning the pattern rather than creating more dirt.

The solution

For the processors

I introduced a new method unstable_requestPipeProcessorsApplication which aims at firing the appliers of a group imperatively.

-apiRef.current.updateColumns([]);
+apiRef.current.unstable_requestPipeProcessorsApplication('hydrateColumns');

For the appliers

Instead of listening to GridEvents.pipeProcessorRegister to re-apply their logic, plugins can now use useGridRegisterPipeApplier to give a callback which will be automatically be called

  • When a processor is registered (equivalent of GridEvents.pipeProcessorRegister)
  • When apiRef.current.unstable_requestPipeProcessorsApplication is called
-const handlePipeProcessorRegister = React.useCallback<
-  GridEventListener<GridEvents.pipeProcessorRegister>
->(
-  (name) => {
-    if (name !== 'hydrateColumns') {
-      return;
-    }
-    const columnsState = createColumnsState({ /* ... */ });
-    setGridColumnsState(columnsState);
-  },
-  [apiRef, logger, setGridColumnsState, columnTypes],
-);
-
-useGridApiEventHandler(apiRef, GridEvents.pipeProcessorRegister, handlePipeProcessorRegister);

+const hydrateColumns = React.useCallback(() => {
+  const columnsState = createColumnsState({ /* ... */ });
+  setGridColumnsState(columnsState);
+}, [apiRef, logger, setGridColumnsState, columnTypes]);
+
+useGridRegisterPipeApplier(apiRef, 'hydrateColumns', hydrateColumns);

#4322 will document in detail this behavior

@flaviendelangle flaviendelangle self-assigned this Apr 6, 2022
@mui-bot
Copy link

mui-bot commented Apr 6, 2022

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 240.6 488.5 329.1 338.38 91.414
Sort 100k rows ms 466.5 1,053.7 725.2 755.64 187.402
Select 100k rows ms 134.5 195.2 183.8 166.8 26.533
Deselect 100k rows ms 97.2 205.9 197.6 173 39.514

Generated by 🚫 dangerJS against 825cf0d

@flaviendelangle flaviendelangle added core Infrastructure work going on behind the scenes component: data grid This is the name of the generic UI component, not the React module! labels Apr 6, 2022
@flaviendelangle flaviendelangle marked this pull request as ready for review April 7, 2022 10:17
Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

No problem pushing this forward. I think we should still avoid calling unstable_requestPipeProcessorsApplication as much as possible, to not cause an additional rerender. The features should be designed to be resilient when the model contains invalid data, e.g. missing columns.

@flaviendelangle
Copy link
Member Author

I think we should still avoid calling unstable_requestPipeProcessorsApplication as much as possible

Yes
And if you have a concrete way of not using it in specific scenario, I am always interested

@flaviendelangle flaviendelangle merged commit ff2d668 into mui:master Apr 11, 2022
@flaviendelangle flaviendelangle deleted the pipe-applier branch April 11, 2022 11:41
@flaviendelangle flaviendelangle mentioned this pull request Apr 13, 2022
15 tasks
alexfauquette pushed a commit to alexfauquette/mui-x that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: data grid This is the name of the generic UI component, not the React module! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants