[DataGrid] Pagination refactoring

[dtassone]

Escape

[oliviertassinari]

Do we even need a public enum for this? Do we even need a private enum for this?

Suggested change

	export enum FeatureMode {
	Client = 'client',
	Server = 'server',
	}
	export enum FeatureMode {
	client = 'client',
	server = 'server',
	}

[dtassone]

it's cleaner and it avoids using magic strings. I need it in several places

[dtassone]

same can't commit this change. Needs to be done across the repo

[oliviertassinari]

Ok, so you advocate for using a variable for internal usage. I used to do that often when using JavaScript only. I assumed that with TypeScript, it would duplicate with the types, making it unnecessary.

Regarding the public API? should we expose FeatureMode as, say GridPaginationFeatureMode? I'm not sure it will be really helpful.

[dtassone]

Well we can expose it and let users decide if they want to use it or not.
I don't see why we should add a prefix like Grid or GridPagination.
I'm planning to use this enum in sorting too and probably filtering.

Users won't have name clashing, as they should use the lib in isolation of other components. It's not like they are going to use all their components in one file. Libraries don't prefix all their types with the library name.

[oliviertassinari]

What about we keep it private? We should be cautious with providing two ways to do the same things, it creates confusion around what's the correct way. It wouldn't be consistent with how we handle the API with the other components. We don't expose variables for each enum the components supports. Does it solve a problem for users?

Regarding prefixing, I think that it's important to consider that each exposed variable is in a global namespace, containing all the components. So far, we have enforced no collison on the exposed modules. I think that we should continue to do so. It's important for 1. people that grep in their codebase for refactoring 2. re-exporting the modules from @material-ui/x.

[oliviertassinari]

It's also important for the documentation, to make it more searchable. It's basically something we started discussing in https://github.com/mui-org/material-ui-pickers/issues/1778

[dtassone]

Well see it a bit like passing a constant instead of a string directly. In the end, it's a string.

Ok for prefixing with 'Grid' if you want. I don't mind.

I thought they would do something a bit like this, so it works with tree shaking

import { FeatureMode } from '@material-ui/x/grid'

[oliviertassinari]

Yeah, I think that Grid would be enough :), mind that we had prefixes for ExpensionPanel too, much longer.

Would you do?

import { Button, ButtonVariant } from '@material-ui/core';

<Button variant={ButtonVariant.outlined}>

[dtassone]

nope I would do something like
<Button variant={theme.variant} />

In the button code, I wouldn't use "variant" as my code would be full of if(something === "variant")
As if "variant" changes then I have to rename it everywhere. Hence constants or enum.

[oliviertassinari]

One item missing, otherwise, all good

[oliviertassinari]

Need to prevent setState if the component unmounts.

So how do you want to do that? Are you sure you want to add this complexity in the demo?

Yes, we especially want to do it if it adds complexity. It's great to feel the pain around it: "here is what it's required to do it right: ok, it's hard, ok maybe we can improve the API to make it easier".

In any case, it's important to showcase best practices. We need the feedback loop.

[oliviertassinari]

We have a couple of options, linking them:

[oliviertassinari]

• active variable: https://github.com/mui-org/material-ui/blob/b2c98cfd4146a8dfcfc4d12fbf6f77a2146bff1a/docs/src/pages/components/autocomplete/GoogleMaps.tsx#L97
• cancel request: https://github.com/mui-org/material-ui-pickers/blob/dfbbe65d7ba4f858a2e660c19d49ab12f5e4fe87/docs/pages/demo/datepicker/ServerRequest.example.jsx#L15

[oliviertassinari]

Actually, outside of the unmounting issue, there is another one that is even more important that I forgot about: concurrent resolution.

If users switch from page 1 to page 2 then quickly to page 3 but the request for page 2 resolve after page 3, we still need to make sure we only consider the data of page 3, not the latest 2. It's not guaranteed by the current logic (if we replace setTimeout with a network request).

[oliviertassinari]

A thought for the coming months, not directly linked to the pull request.

Actually, I'm confused, for server-side pagination, won't we need a different API? I don't see how this once can scale to grouping, filtering, sorting. Maybe we don't need the server-side pagination, but only a server-side data fetcher (that would solve the unmounting and concurrency issues directly + a load more like windowing).

[dtassone]

Well RxJs is massively popular in react as well. Angular is built on it to manage events...
In react, events are managed by react, and server requests are out of the react scope.

That being said, in a react app it's up to you, how you build your communication layer with the server. You can use axios, fetch or rxjs if you want.
This is what makes react great and this is also why ppl go with angular as they want a full layer architecture and not just the component or rendering layer that react provides

[oliviertassinari]

The proposed approach would fix two bugs with the current demo that people will face as soon as they copy the demo in their app and replace the fetch with a real network request.

Will it make the demo more complex? Not sure. It could be simpler to control the active page.

[dtassone]

Ok I do it

[dtassone]

Done

[dtassone]

Do you want to manage server errors in this demo as well? Maybe add a catch with a server boundary, or we can add an error overlay in the grid which I think would be great