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] Fix valueGetter sorting #348

Merged
merged 9 commits into from
Sep 24, 2020
Merged

Conversation

dtassone
Copy link
Member

No description provided.

@oliviertassinari oliviertassinari changed the title [DataGrid] fix valueGetter sorting [DataGrid] Fix valueGetter sorting Sep 24, 2020
@oliviertassinari oliviertassinari added the docs Improvements or additions to the documentation label Sep 24, 2020
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Ok so the approach is to fix the demo in the documentation and we will wait to consider altering the RowModel object with the valueGetter() returned value.

Comment on lines 5 to 6
const getFullName = (params: ValueGetterParams) =>
`${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;
Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020

Choose a reason for hiding this comment

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

I think that we have to stick to a convention between arrow functions and named function. The current one is named function when possible, e.g. for the top-level scope and arrow functions for nested scopes, when inlining is simpler.

Suggested change
const getFullName = (params: ValueGetterParams) =>
`${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;
function getFullName(params: ValueGetterParams) {
return `${params.getValue('firstName') || ''} ${params.getValue('lastName') || ''}`;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Up to you. I prefer the simple and concise syntax of arrow functions.

Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020

Choose a reason for hiding this comment

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

Ok, time for a conversation about it then. @mui-org/core-team.

The problem:
There are two ways to write functions: arrow or named. Defining one convention could avoid noise during refactorization (depending on who writes which line), increase consistency (easier to browse codebase) and avoid having to discuss it in reviews.

The question to answer:
When should we use named function vs arrow function? Should we even care? Meaning leaving it up to what the person prefers to be when he writes a line of code or refactor one.
For instance, right now, the styles object is always an arrow function when components are always (or almost) a function.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well the main problem of named function is that they lose the context. And the syntax is more verbose.
If you define a class in TS, then it's better to use named function as they get attached to the prototype.
if it's a function component then arrow functions are fine.

I think it's important to be consistent.

Copy link
Member

Choose a reason for hiding this comment

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

If it doesn't matter we shouldn't enforce either style if it's not auto-fixable. If a lint rule can find and fix it we can configure it however you like. But I don't want to argue every time why I prefer one over the other and I don't want reviewers wasting their time nitpicking code style. There's enough substance to review.

Copy link
Member

Choose a reason for hiding this comment

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

And the syntax is more verbose.

Only if you use implicit return. Once you have to use explicit return (and this is something I end up using anyway once you have to debug) the function declaration actually requires one less character.

Another type related downside: const arrow function expressions can't be generic.

I think it's fairly obvious that there isn't a rule to use one or the other. Eventually you have to use function declarations anyway so why disallow them?

Copy link
Member

@oliviertassinari oliviertassinari Sep 24, 2020

Choose a reason for hiding this comment

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

Ok, so we are leaning toward: no rules, use whatever you find best in the given context?

dtassone and others added 3 commits September 24, 2020 16:55
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants