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] Should not expose apiRef #1821

Closed
2 tasks done
oliviertassinari opened this issue Jun 3, 2021 · 6 comments · Fixed by #2312
Closed
2 tasks done

[DataGrid] Should not expose apiRef #1821

oliviertassinari opened this issue Jun 3, 2021 · 6 comments · Fixed by #2312
Assignees
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module!
Milestone

Comments

@oliviertassinari
Copy link
Member

oliviertassinari commented Jun 3, 2021

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

Developers can access apiRef in the community plan, while we document it a Pro plan feature.

Expected Behavior 🤔

Developers shouldn't be able to access apiRef in DataGrid, only in XGrid.

Steps to Reproduce 🕹

Steps:

  1. https://codesandbox.io/s/material-demo-forked-59fsr?file=/demo.tsx
export default function DataGridDemo() {
  return (
    <div style={{ height: 400, width: "100%" }}>
      <DataGrid
        onRowSelected={(parmas) => {
          console.log(Array.from(parmas.api.current.getSelectedRows().entries()));
        }}
        rows={rows}
        columns={columns}
        pageSize={5}
        checkboxSelection
      />
    </div>
  );
}

Context 🔦

Saw it in #1820

As a side note, the current types and naming looks strange:

  1. Why aren't we reducing the types to GridApiRef instead of any?

https://github.com/mui-org/material-ui-x/blob/fa346f0fbe3d9b9eea9bb403fe4675f544d6abf9/packages/grid/_modules_/grid/models/params/gridRowSelectedParams.ts#L15-L18

  1. Why are naming the prop api if we don't provide GridApi? Shouldn't it be called apiRef?

  2. Why expose apiRef when we could provide the api directly?

Your Environment 🌎

4.0.0-alpha.30

@oliviertassinari oliviertassinari added breaking change component: data grid This is the name of the generic UI component, not the React module! labels Jun 3, 2021
@dtassone
Copy link
Member

dtassone commented Jun 3, 2021

Why aren't we reducing the types to GridApiRef instead of any?

due to circle dependencies

Why are naming the prop api if we don't provide GridApi? Shouldn't it be called apiRef?

Yes there is an inconsistency in that, originally I wanted to provide apiRef.current so it's a bit nicer for the user but I think it can create issues.

Why expose apiRef when we could provide the api directly?

If we expose api then the state of the api that was attached to current at the time we pass might be outdated if we use it outside a function or a useEffect. Hence why we use a ref.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 3, 2021

@dtassone Ok, so it would only help if we rename the property provided in the params from api -> apiRef?

@dtassone
Copy link
Member

dtassone commented Jun 3, 2021

yes for consistency everywhere.

@oliviertassinari
Copy link
Member Author

If we expose api then the state of the api that was attached to current at the time we pass might be outdated if we use it outside a function or a useEffect. Hence why we use a ref.

@dtassone Are you saying that the api and its methods are not stable by referential comparison? I thought we fixed this already in: https://github.com/mui-org/material-ui-x/blob/c992eb3de48ed3f465cb8d3daacd45f13758224a/packages/grid/_modules_/grid/hooks/root/useGridApiMethod.ts#L23

@dtassone
Copy link
Member

dtassone commented Jun 3, 2021

The methods should be stable. But the state might be outdated if you use api in a useEffect for example as if there is a cycle then the ref won't be updated.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 3, 2021

Actually, based on #1103 (comment) maybe we should always exposes the apiRef but limits the methods available based on the version. We can implement this with module augmentation.

The other use case for the apiRef with the DataGrid is customizations, I believe that the apiRef is required to bind the events correctly, for instance, with a custom pagination: https://master--material-ui-x.netlify.app/components/data-grid/components/#pagination

import * as React from 'react';
import { makeStyles } from '@material-ui/styles';
import { DataGrid, useGridSlotComponentProps } from '@material-ui/data-grid';
import { useDemoData } from '@material-ui/x-grid-data-generator';
import Pagination from '@material-ui/lab/Pagination';

const useStyles = makeStyles({
  root: {
    display: 'flex',
  },
});

function CustomPagination() {
  const { state, apiRef } = useGridSlotComponentProps();
  const classes = useStyles();

  return (
    <Pagination
      className={classes.root}
      color="primary"
      count={state.pagination.pageCount}
      page={state.pagination.page + 1}
      onChange={(event, value) => apiRef.current.setPage(value - 1)}
    />
  );
}

export default function CustomPaginationGrid() {
  const { data } = useDemoData({
    dataSet: 'Commodity',
    rowLength: 100,
    maxColumns: 6,
  });

  return (
    <div style={{ height: 400, width: '100%' }}>
      <DataGrid
        pagination
        pageSize={5}
        components={{
          Pagination: CustomPagination,
        }}
        {...data}
      />
    </div>
  );
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: data grid This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants