-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
[core] Add typing to the pre-processors methods #3595
[core] Add typing to the pre-processors methods #3595
Conversation
type GridPreProcessorsApplierArg< | ||
P extends GridPreProcessingGroup, | ||
T extends { value: any }, | ||
> = T extends { context: any } ? [P, T['value'], T['context']] : [P, T['value']]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In https://github.com/mui-org/material-ui-x/pull/3595/files#diff-b1ecb3ac2bbd884ca95d8879603f25ff6f844a259f0ceb6207fb06682cc933f6R37 you changed to accept several params. How do you plan to type if we add a pre-processor that has more than 3 arguments in the apiRef.current.unstable_applyPreProcessors
call.
const canBeReorderedProcessed = apiRef.current.unstable_applyPreProcessors(
GridPreProcessingGroup.canBeReordered,
canBeReordered,
{ targetIndex: targetColVisibleIndex },
hasMovedLeft,
hasMovedRight
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a hack to make TypeScript accept both 1 "params" and 0 "params" without doing something like [value, params] = args as any
.
But if it's not clear, maybe the as any
it better.
These are the results for the performance tests:
|
Extracted from #3593
As it do not impact the users at all, I will automerge 7 days after the review request.
I think other PRs are more worth spending time reviewing 馃憤