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] Use pre-processors for sorting and filtering #3318

Merged

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 2, 2021

Extracted from #3277
Part of #2994

Behavior

The sorting and filtering pre-processor do not work exactly as the column one.

Behavior of the colums pre-processor

  1. useFeatureXXX calls registerPreProcessor because one of the prop used in some updateFeatureXXXColumn has changed due to a change in prop.featureXXXMode (for instance).
  2. useGridColumns receives a preProcessorRegister event.
  3. useGridColumns calls applyPreProcessors with the unprocessed columns.
  4. useGridPreProcessing calls all the colums pre-processor in the registration order, each pre-processor add / remove or update some columns.
  5. useGridPreProcessing returns the pre-processed columns

Imagine that useFeatureXXX is called in the DataGridPro but only wants to add a column if prop.featureXXX is enabled.
Since useFeatureXXX is called even when prop.featureXXX = false, its method updateFeatureXXXColumn will still change its reference when prop.featureXXXMode changes.
So when prop.featureXXXMode changes, we will re-apply all the column pre-processors "uselessly".

For the columns, it is not a major issue since the prep-processing are very light. With the new format passing the GridColumnsRawState, the complexity to check if a column needs to be added / removed is usually constant thanks to the lookup.

But for the sorting / filtering, we really don't want to re-apply the sorting / filtering when prop listened by a disabled feature are updated.

Behavior of the sorting / filtering pre-processor

The logic below describes the sorting pre-processor but things are exactly the same for the filtering one.

  1. useFeatureXXX calls registerPreProcessor because one of the prop used in some updateFeatureXXXColumn has changed due to a change in prop.featureXXXMode (for instance).
  2. useGridSorting receives a preProcessorRegister event.
  3. useGridSorting calls applyPreProcessors with an empty object.
  4. useGridPreProcessing calls all the sorting pre-processor in the registration order, each pre-processor add a property to the empty object with its treeGroupingName as a key and its sorting algorithm as a value.
  5. useGridPreProcessing returns the collection of sorting algorithms.
  6. useGridSorting stores the collection in a ref.
  7. useGridSorting checks if the sorting algorithm matching the current treeGroupingName has changed since the last sorting application, if so it re-applies the sorting, if not it don't re-apply the sorting.

This has two main behavior difference:

  • If I register a new treeData sorting method but I am in treeGroupingName = "groupingColumns", I won't re-apply the sorting
  • I only call the sorting method matching the algorithm used to build the tree currently in state

Design choices

I chose to re-use apiRef.current.unstable_applyPreProcessors and apiRef.current.unstable_registerPreProcessor to avoid increasing the API surface. But to be compatible with it I had to add a ref in the sorting and filtering hook to store the algorithm collections.
A cleaner implementation would probably be possible by creating two types of pre-processors:

Chained pre-processors:
A hook listens to preProcessorChainRegister and preProcessorChainUnRegister
These events each time a pre-processor is added / removed / changed.
Whenever one of these events is fired, the hook calls applyPreProcessorChain with an input, the method calls all the pre-processors and return an ouput of the same shape.

Branch pre-processor:
A hook listens to preProcessorBranchChanged
These events each time a pre-processor is added / removed / changed.
Whenever the branch (for sorting / filtering the branch would be state.rows.treeGroupingName changes) AND whenever this even is fired, the hook calls getPreProcessorBranch with the current branch, the method returns the algorithm matching this branch.
The hook then compare this method with the method applied the last time he ran the logic (the last time useGridSorting ran applySorting for instance), if the method has changed, it re-apply the logic, if not it does nothing.

BUT I prefer to wait before adding a new abstraction. Pre-processor are a powerfull tools with new use-cases coming rapidly. I prefer to take a step back and wait for more use-cases to build an abstraction better fit for all of them.
For now, handling the algorithm collection in useGridSorting and useGridFiltering causes code duplication but is not big of an issue.

@flaviendelangle flaviendelangle self-assigned this Dec 2, 2021
const groupingColDefField = GRID_TREE_DATA_GROUP_COL_DEF_FORCED_PROPERTIES.field;

const shouldHaveGroupingColumn = props.treeData;
const haveGroupingColumn = columnsState.lookup[groupingColDefField] != null;
Copy link
Member Author

Choose a reason for hiding this comment

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

As written in my PR description, for the column pre-processing, the check to know if we need to do something has a complexity of O(1).
The only linear complexity here is if we want to remove the grouping column (to find its index) but iit won't be need if the prep-processor is called after a change in a prop of another pre-processor.

@@ -404,6 +335,37 @@ export const useGridFilter = (
'FilterApi',
);

/**
Copy link
Member Author

Choose a reason for hiding this comment

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

I started to structure the big hooks with section comments to make it easier to find something.

apiRef.current.setFilterModel(props.filterModel);
}
}, [apiRef, logger, props.filterModel]);
const handlePreProcessorRegister = React.useCallback<
Copy link
Member Author

Choose a reason for hiding this comment

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

See description of the PR for more details about this logic

import { GridPreProcessingGroup, useGridRegisterPreProcessor } from '../../core/preProcessing';
import { GridApiRef } from '../../../models';

export const useGridRegisterFilteringMethod = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Small abstraction of the collection update behavior.
It does not bring a lot of value but without it, the code is useGridTreeData / useGridGroupingColumns was less readable.

* - One of its children is passing the filter
* - It is passing the filter
*/
export const filterRowTreeFromTreeData = (
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy / paste from useGridFilter
The filtering behavior between useGridTreeData and useGridGroupingColumns is to different to have the same method.

sortRowList: (rowList: GridRowTreeNodeConfig[]) => GridRowId[];
}

export const sortRowTree = (params: SortRowTreeParams) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy / paste from useGridSorting
The behavior is the same in useGridGroupingColumns so I set this method in an util folder to use it for both features.

@flaviendelangle flaviendelangle added the core Infrastructure work going on behind the scenes label Dec 2, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 3, 2021
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 3, 2021
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 3, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 6, 2021
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 7, 2021
@github-actions
Copy link

github-actions bot commented Dec 7, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 7, 2021
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.

A cleaner implementation would probably be possible by creating two types of pre-processors:

The pre-processor implements the pipe-filter pattern. What we need is another kind of abstraction implementing the strategy pattern.

@github-actions
Copy link

github-actions bot commented Dec 8, 2021

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 8, 2021
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 8, 2021
@flaviendelangle flaviendelangle merged commit 4ff595d into mui:master Dec 8, 2021
@flaviendelangle flaviendelangle deleted the prep-processor-filter-sorting branch December 8, 2021 14:58
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants