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

Introduce Data providers #2644

Merged
merged 93 commits into from
Oct 6, 2023
Merged

Introduce Data providers #2644

merged 93 commits into from
Oct 6, 2023

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Sep 7, 2023

Screen.Recording.2023-09-25.at.14.40.43.mov

To do:

  • Add a switch in the property controls to switch between row mode and dataprovider mode
  • documentation
  • test
  • reset cursor when pageSize changes
  • ?

Documentation:
https://deploy-preview-2644--mui-toolpad-docs.netlify.app/toolpad/concepts/data-providers/
https://deploy-preview-2644--mui-toolpad-docs.netlify.app/toolpad/reference/api/create-data-provider/

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 8, 2023
@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Sep 8, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 11, 2023
This is the strategy your data is paginated by when it returns data based on a cursor and a page size. The `getRecords` method will receive `cursor` and `pageSize` values in it `paginationModel` parameter and returns a set of records representing the page. You indicate the cursor of the next page with a `cursor` property in the result. Pass `null` to signal the end of the collection. The You can enable Cursor based pagination by setting `paginationMode` to `'cursor'`.

```tsx
export default createDataProvider({
Copy link
Member

@apedroferreira apedroferreira Oct 5, 2023

Choose a reason for hiding this comment

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

Maybe i'm not super familiar with cursor pagination, but it isn't completely obvious to me right away how to use this cursor pagination.
is the cursor a number? Or can I use the id of a record?
The prisma documentation on this kind of thing is very clear, maybe we could use it as inspiration https://www.prisma.io/docs/concepts/components/prisma-client/pagination.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, also the way Prisma does it kind of just works and we don't have to explicitly choose a pagination model. Not sure if we could do something similar?

Copy link
Member

@apedroferreira apedroferreira Oct 5, 2023

Choose a reason for hiding this comment

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

Nevermind I understand this is quite different from Prisma now as it integrates very directly with the Data Grid, these were just some quick ideas while going over the docs and trying to use the feature in a basic way. Feel free to ignore if they don't make sense or don't provide any insights, I'm sure you looked at these problems deeper than me :D

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've added another paragraph explaining the value of cursor in this pagination model.

Oh, also the way Prisma does it kind of just works and we don't have to explicitly choose a pagination model. Not sure if we could do something similar?

In prisma you don't need to explicitly specify a pagination model because it doesn't need to signal to consumers how it is to be called. The Toolpad data provider needs to signal to Toolpad how it's to be fetched. Supplying this option configures the interfaces in getRecords for the right pagination model.

Janpot and others added 5 commits October 5, 2023 19:19
Co-authored-by: Pedro Ferreira <10789765+apedroferreira@users.noreply.github.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Co-authored-by: Pedro Ferreira <10789765+apedroferreira@users.noreply.github.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Co-authored-by: Pedro Ferreira <10789765+apedroferreira@users.noreply.github.com>
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

Looks good and it's easy to use, really great feature! Here's some feedback I collected while trying it out, just trying to give as much feedback as possible:

  • I set up some static data for records but forgot to include id strings, so an error message showed in the Data Grid telling me to do that. Once I included the ids and saved the file, the Data Grid did not update without the error until I refreshed the page.
  • Maybe we could leave the cursor choice for later if possible instead of having it in the modal right away, and start with index pagination automatically? But this could depend on many things, it's just a thought to reduce initial friction/confusion a bit if someone's trying the feature for the first time.
  • If I'm using the data grid for the first time, I notice these options and I try to create a data provider, how can I quickly get to the documentation so I can understand all the options I have? As this can be a complex feature, maybe we can make this easier. Maybe we can have a comment with a link to the docs in the data provider template (and eventually more helpful comments in general).

createProviderMutation.mutate();
};

const nameExists = existingNames.has(newName);
Copy link
Member

Choose a reason for hiding this comment

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

Why not just check this inside the errorMessage function itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

nameExists is a boolean and existingNames is a Set. Therefore nameExists is automatically stable, existingNames depends on the consumer to provide a stable value. .has is a fast operation. Not sure which is better, but don't believe it matters much. I can put it inside as well.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, it's not important so we can keep it, I think it's fine either way.

onClose={handleClose}
onCommit={(newName) => onChange(`${newName}.ts:default`)}
initialName={dialogValue}
existingNames={existingNames}
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it would be better to pass a validation function instead, anyway not a big deal.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 6, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 6, 2023
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 6, 2023
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 6, 2023
@Janpot
Copy link
Member Author

Janpot commented Oct 6, 2023

  • I set up some static data for records but forgot to include id strings, so an error message showed in the Data Grid telling me to do that. Once I included the ids and saved the file, the Data Grid did not update without the error until I refreshed the page.

I added an error boundary to the grid. Also adding ids automatically when data provider is used, just the way it is done when rows prop is used. This also uncovered some problems around column inference in the columns editor which had the same root cause

  • Maybe we could leave the cursor choice for later if possible instead of having it in the modal right away, and start with index pagination automatically? But this could depend on many things, it's just a thought to reduce initial friction/confusion a bit if someone's trying the feature for the first time.

The choice of pagination mode is made in the code only. It's not an option that can be easily changed from the UI. This initializes the code with the right pagination mode. Initially this wasn't part of data providers but was suggested in the last grooming call.

  • If I'm using the data grid for the first time, I notice these options and I try to create a data provider, how can I quickly get to the documentation so I can understand all the options I have? As this can be a complex feature, maybe we can make this easier. Maybe we can have a comment with a link to the docs in the data provider template (and eventually more helpful comments in general).

Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

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

The changes look great! Thanks for addressing the feedback.

@Janpot Janpot merged commit c8aa712 into mui:master Oct 6, 2023
11 checks passed
@Janpot Janpot deleted the data-providers branch October 6, 2023 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants