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] Deprecate the getValue param for the valueGetter callback #3314

Merged
merged 3 commits into from
Dec 6, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Dec 1, 2021

Fixes #1564
Extracted from #3277

For #3277 I need to call the valueGetter before the column state is initialized to use its value as a grouping key.
But at that point I can't use getBaseCellParams in useGridParamsApi because many api methods / states are not ready yet.
I added a GridValueGetterSimpleParams interface and make valueGetter callable with either the full params or this new interface.

It is not a breaking change since the new incomplete interface is only called for new code in #3277.
But I migrated all our examples to be compatible with #3277 and the main friction point is the getValue param which is an apiRef method that I can't use before useGridParamsApi initialization.

We are using this property in 2 callbacks:

  • getRowClassName: I can understand the interest of having a wrapper that given a field, handles the potential valueGetter.

  • valueGetter: I don't really see the point of using this property instead of a direct row access. The main difference is that if a col A uses getValue('B') in its valueGetter and if the col B has itself a valueGetter, it will call it. But I think this valueGetter chain adds a lot of not very usefull complexity and that we should deprecate the getValue for the valueGetter callback.

@flaviendelangle flaviendelangle self-assigned this Dec 1, 2021
@flaviendelangle flaviendelangle added the component: data grid This is the name of the generic UI component, not the React module! label Dec 1, 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.

Wouldn't be simpler to not create new interfaces and add a warning when getValue is called?

valueGetter: I don't really see the point of using this property instead of a direct row access.

It's useful for when the row structure is nested.

@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
@flaviendelangle
Copy link
Member Author

Wouldn't be simpler to not create new interfaces and add a warning when getValue is called?

I'm only removing getValue for valueGetter so I'm not sure how we could do this, could you elaborate please ?

It's useful for when the row structure is nested.

Would you have an example for valueGetter where it would as useful as possible ?

@m4theushw
Copy link
Member

I'm only removing getValue for valueGetter so I'm not sure how we could do this, could you elaborate please ?

I was saying to not create new interfaces and only add a warning if getValue is called.

Would you have an example for valueGetter where it would as useful as possible ?

Are you talking about valueGetter or getValue? I was referring to the first in #3314 (review) to say that it's useful when the data is nested. However, getValue is redundant and can be removed in the major version.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Dec 3, 2021

Are you talking about valueGetter or getValue?

I am talking about the getValue passed as a param to valueGetter.
When it is passed as a param for getRowClassName I don't see any problem. I don't think it's a crucial feature but it does not bother me.
But for valueGetter, it creates chained valueGetter calls and I don't think it's a good idea. Especially with #3277 where it can't have it when calling valueGetter for the grouping process.

/**
* Parameters passed when calling `colDef.valueGetter` in the row grouping sequence.
*/
export interface GridValueGetterSimpleParams<R = any> {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's worth to create another interface to remove getValue. In v6 we'll need to remove it. Instead, we could only flag it as deprecated and be cautious to never call it + display warning if someone calls it.

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 interface has other differences than getValue
Once again it's for the grouping process, so it does not have api or the full rowNode either for instance.

If you want to fully deprecate getValue, I can move it's deprecation from GridValueGetterFullParams to GridCellParams. That was not the goal of this PR but I'm not agaist the idea.
But for GridValueGetterSimpleParams I think we do need another interface.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to fully deprecate getValue, I can move it's deprecation from GridValueGetterFullParams to GridCellParams. That was not the goal of this PR but I'm not agaist the idea.

Yes, let's deprecate getValue entirely. The sooner we do, less work we have for v6.

But for GridValueGetterSimpleParams I think we do need another interface.

For now, it's necessary because the api is a required property. However, in v6 this interface can be removed.

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.

I'm approving to make #3277 to move faster.

Two things which could be done latter:

/**
* Parameters passed when calling `colDef.valueGetter` in the row grouping sequence.
*/
export interface GridValueGetterSimpleParams<R = any> {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to fully deprecate getValue, I can move it's deprecation from GridValueGetterFullParams to GridCellParams. That was not the goal of this PR but I'm not agaist the idea.

Yes, let's deprecate getValue entirely. The sooner we do, less work we have for v6.

But for GridValueGetterSimpleParams I think we do need another interface.

For now, it's necessary because the api is a required property. However, in v6 this interface can be removed.

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

InternalError too much recursion only when GridColDef.valueGetter is included
2 participants