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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Limit access to the apiRef methods depending on the plan and the calling place #3166

Closed
flaviendelangle opened this issue Nov 12, 2021 · 12 comments
Assignees
Labels
component: data grid This is the name of the generic UI component, not the React module! discussion RFC Request For Comments v6.x

Comments

@flaviendelangle
Copy link
Member

flaviendelangle commented Nov 12, 2021

Context: 馃И Notion: apiRef MIT vs. Commercial

Current behavior

Note that the following examples describes what is theoretically possible to do.
Some examples shows how to use undocumented internals of the grid, this should never be done and could produce unwanted behaviors.

User code with DataGridPro

Outside of the scope of the grid

Users can initialize the apiRef with useGridApiRef and pass it to the DataGridPro as a prop.
They can then use all the methods of apiRef as long as they do it after the first render.

const MyDataGrid = (otherProps) => {
  const apiRef = useGridApiRef();
  
  React.useEffect(() => {
    // Works
    apiRef.current.subscribeEvent(GridEvents.rowsScroll, () => console.log('HELLO WORLD'));
  
    // Works
    apiRef.current.unstable_eventManager.removeAllListeners();
  }, [apiRef]);
  
  React.useLayoutEffect(() => {
    // Works
    apiRef.current.subscribeEvent(GridEvents.rowsScroll, () => console.log('HELLO WORLD'));
  
    // Works
    apiRef.current.unstable_eventManager.removeAllListeners();
  }, [apiRef]);
  
  // Crashes: TypeError: apiRef.current.subscribeEvent is not a function
  useGridApiEventHandler(apiRef, GridEvents.rowsScroll, () => console.log('HELLO WORLD'));
  
  return <DataGridPro apiRef={apiRef} {...otherProps} />
};

Inside the scope of the grid

Users can access the apiRef with useGridApiContext.
They can then use all the methods of apiRef.

const MyCustomFooter = (props) => {
  const apiRef = useGridApiContext();
  
  React.useEffect(() => {
    // Works
    apiRef.current.subscribeEvent(GridEvents.rowsScroll, () => console.log('HELLO WORLD'));
  
    // Works
    apiRef.current.unstable_eventManager.removeAllListeners();
  }, [apiRef]);
  
  React.useLayoutEffect(() => {
    // Works
    apiRef.current.subscribeEvent(GridEvents.rowsScroll, () => console.log('HELLO WORLD'));
  
    // Works
    apiRef.current.unstable_eventManager.removeAllListeners();
  }, [apiRef]);
  
  // Works
  useGridApiEventHandler(apiRef, GridEvents.rowsScroll, () => console.log('HELLO WORLD'));
  
  return <GridFooter {...props} />
}

const MyDataGrid = (otherProps) => {
  return <DataGridPro {...otherProps} components={{ Footer: MyCustomFooter }} />
};

User code with DataGrid

Outside of the scope of the grid

Users can't pass the apiRef prop to the DataGrid and can therefore not use any apiRef feature.

Inside the scope of the grid

Users can access the apiRef with useGridApiContext.
They can then use all the methods of apiRef.
See the examples of the DataGridPro above.

Problems with the current behavior

We are exposing all our internals

We started prefixing our internals with unstable_ and we do not document them.
But they are still accessible and auto-completed on people IDEs.
Ideally, methods that should never be used outside of the package code should not be accessible in any way outside of the package.

The access to the apiRef is inconsistent on the DataGrid

Our documentation indicates that the apiRef is only available for DataGridPro.
This is for instance made explicit with the pro icon on this paragraph.
Yet, this example is using the apiRef on the DataGrid and is fully valid.

The access to the apiRef should not depend on where the user code is.

Proposals

Create a privateApiRef

Concept

Have two apiRef instead of a single one.

  1. publicApiRef is created in useGridApiRef, it only contains the public methods of the current plan and is accessible with useGridApiContext
  2. privateApiRef is created in useGridApiInitialization, it contains both the public and the private API, is used in all our feature hooks and in the components not overridable with component slots, it can be accessed with useGridPrivateApiContext

Note that all the plan would use the publicApiRef, with a content that changes according to the plan if we want to set some method public only on the pro and / or premium plan.

Implementation example

You can find a POC on my fork flaviendelangle#11 that shows how we could set private methods on the apiRef.

There are probably other solutions that we can explore, the goal here is only to show that if we want to have limit what is exposed, it can be achieved without splitting the apiRef because both the public and the private apiRef includes all the public methods

Dynamically set a method to be public / private

// useGridEditRows.ts
apiRef.current.registerMethod(
  'commitCellChange, 
  props.signature === GridSignature.DataGridPro,
  commitCellChange
);

Usage of public methods

Just like today. useGridApiContext would return the public API not wrapped in the proxy.

Usage of private methods

In the feature hooks, we would pass the wrapped apiRef so it would always have access to the private apis.

In the components, if we are using a method that is private or that can be private depending on the plan wee would use a new useGridPrivateApiContext that returns the public API wrapped in the proxy.

Open the apiRef to the users of the DataGrid

The team is in favor of letting users of the DataGrid pass an apiRef prop to the grid, just like on the DataGridPro.
The following example would then be possible:

const MyDataGrid = (otherProps) => {
  const apiRef = useGridApiRef();
  
  React.useEffect(() => {
    apiRef.current.subscribeEvent(GridEvents.pageSizeChange, () => console.log('HELLO WORLD'));
  }, [apiRef]);
  
  return <DataGrid apiRef={apiRef} {...otherProps} />
};

What would still be blocked on the free plan

Methods of pro-only features

Whatever solution we decide to implement. the methods added to the apiRef by hooks only present on the DataGridPro would not be accessible on the DataGrid. Even in our internal code they are not added to the apiRef.

For instance #2946 in adding a apiRef.current.pinColumn in the new useGridColumnPinning hook. This hook is not called on the DataGrid so the method is never added to the apiRef for the DataGrid.

Forced props would still apply

Giving access to methods like apiRef.current.setFilterModel to users of the DataGrid will not bypass the fact that the DataGrid can only set one filter item. If you try to pass several items to apiRef.current.setFilterModel, you will get an error.

Some methods will have to be refactored.
For instance apiRef.current.setSelectionModel is not applying the limitation.
But it is a lot more consistent to do it since with the current approach, a user of the DataGrid could rightfully call apiRef.current.setSelectionModel with several rows inside a custom component and it would work.

What if we want to limit access to a method to the pro plan

There are a few methods that we are still considering limiting.
The main candidates are all the methods around the editing (apiRef.current.commitCellChange, apiRef.current.commitRowChange, apiRef.current.setEditCellValue mainly).

The following paragraphs explore what it would mean to block some arbitrary methods on the free plan. Even if we decide that the methods above should not be blocked, the global ideas would still apply to any future method we only want to expose on a given plan and that is created by hooks also called on a lower plan.
Therefore, the examples below uses very basic methods to illustrate the issue.

In those examples, we also take for granted that we implemented the proposal splitting the public and private apiRef.

If the method is used in components overridable by component slots

Imagine that we want to block the method apiRef.current.setPageSize in the free plan.
We could not take for granted that useGridApiRef contains the method setPageSize and should use the privateApiRef to access it in our cross-plan components.
A simplified version of our GridPagination would then look something like:

const GridPagination = () => {
  const privateApiRef = useGridPrivateApiContext();
  const rootProps = useGridRootProps();
  const paginationState = useGridSelector(apiRef, gridPaginationSelector);

  return (
    <TablePagination 
      count={paginationState.rowCount}
      page={paginationState.page}
      rowsPerPageOptions={rootProps.rowsPerPageOptions}
      onPageChange={(e, page) => privateApiRef.current.setPage(page)}
      onRowsPerPageChange={(e) => privateApiRef.current.setPageSize(Number(e.target.value)}
    />
  )
}

But if users want to use the Pagination component slot. They could do it, but they could never truly build a working component since they would not have access to useGridPrivateApiContext

I think we should only block access based on the plan if those methods are never used on a component that can be overrided as a slot.

@flaviendelangle flaviendelangle added component: data grid This is the name of the generic UI component, not the React module! discussion components: DataGridPro labels Nov 12, 2021
@flaviendelangle flaviendelangle self-assigned this Nov 12, 2021
@flaviendelangle
Copy link
Member Author

I will improve the example later 馃憤

@DanailH
Copy link
Member

DanailH commented Nov 29, 2021

We need to account for cases like the one mentioned here #3154.

In short - if you control the pageSize and a the same time call apiRef.current.setPageSize and pass in a value > 100 the grid will throw.

@m4theushw
Copy link
Member

There are a few methods that we are still considering limiting.
The main candidates are all the methods around the editing (apiRef.current.commitCellChange, apiRef.current.commitRowChange, apiRef.current.setEditCellValue mainly).

Taking these methods as example, apiRef.current.commitCellChange and apiRef.current.setEditCellValue are used to build custom edit components. We can't block them on the free version. I would prefer to not pass the API as an api prop to edit components nor to export useGridApiContext context. We could pass, as props, only the methods we think users should use to override components. Doing this we call still call internally apiRef.current.commitCellChange in the free plan, but users don't have access to it in the public apiRef.

const GridPagination = ({ state, setPage, setPageSize }) => {
  const rootProps = useGridRootProps();
  const paginationState = gridPaginationSelector(state);

  return (
    <TablePagination 
      count={paginationState.rowCount}
      page={paginationState.page}
      rowsPerPageOptions={rootProps.rowsPerPageOptions}
      onPageChange={(e, page) => setPage(page)}
      onRowsPerPageChange={(e) => setPageSize(Number(e.target.value)}
    />
  )
}

I think we should only block access based on the plan if those methods are never used on a component that can be overrided as a slot.

I exposed above but I prefer to not pass the api prop. For future features, we design them having in mind what should be exposed to users from the free version.

@flaviendelangle
Copy link
Member Author

nor to export useGridApiContext context

I don't see the interest of totally removing the useGridApiContext from the free version.
It would be a major feature removed from this version and at the same time we would give them access to the same API outside of the scope of the Grid.

Couldn't we let useGridApiContext accessible, make some methods private on the free plan and public on the pro plan (commitCellChange for instance).
And if some of those methods are used in a component slot, then we pass them as prop to avoid making those slots unusable.

We could always pass the apiRef methods usefull in a component as slot (like in your example with GridPagination above).
It would probably be more straightforward for light customization. But I don't think we necessarily have to do it at the same time as we open the useGridApiRef to the free plan.

@m4theushw
Copy link
Member

m4theushw commented Dec 2, 2021

If I understood, useGridApiContext gives the full API, as it's now. To make harder for users to hijack the grid and get full access to the private API, we should start by not exporting useGridApiContext on the free version. To still use the API inside a slot component, users have the ref they received when called useGridApiRef before instantiating the grid component.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Dec 2, 2021

To make harder for users to hijack the grid and get full access to the private API, we should start by not exporting useGridApiContext on the free version

With my proposal, useGridApiContext would not leak the private API.
It contains the exact same ref as returned by useGridApiRef, the private method are never passed to this ref.

@m4theushw
Copy link
Member

m4theushw commented Dec 2, 2021

Ok, so then useGridPrivateApiContext shouldn't be exported. Correct?

@flaviendelangle
Copy link
Member Author

Yes, neither on the pro, free or premium
On any plan:

publicApi = methods accessible by users on the current plan, it is what is returned by useGridApiRef and useGridApiContext

privateApi = all the methods, used internally, it is what is returned by useGridPrivateApiContext. It is a Proxy that uses an internal object for private-only methods and fallback to the publicApi.

@m4theushw
Copy link
Member

For me sounds good. We only need to consider that if we want to block a method, e.g. commitCellChange, in the free version while still allowing it to be used inside a slot or a custom edit component, it has to be passed as prop, because the apiRef from useGridApiContext won't include it.

For the implementation, we could have a single context storing both all methods and only the public ones. The useGridApiContext becomes the proxy and only allows to call public methods, or private methods if the user has Pro. In the other side, useGridPrivateApiContext allows to call everything.

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Dec 2, 2021

We only need to consider that if we want to block a method, e.g. commitCellChange, in the free version while still allowing it to be used inside a slot or a custom edit component, it has to be passed as prop, because the apiRef from useGridApiContext won't include it.

Fully agree on that.

For the implementation, we could have a single context storing both all methods and only the public ones. The useGridApiContext becomes the proxy and only allows to call public methods, or private methods if the user has Pro. In the other side, useGridPrivateApiContext allows to call everything.

With my proposal, you can hide private methods even on the pro when accessed outside of the Grid scope (with the apiRef returned by useGridApiRef).
It allows to hide all our internal (the unstable_ stuff).
And in the future we have some "premium-only" methods it would be handled (hope we won't do that though).

@oliviertassinari oliviertassinari added the plan: Pro Impact at least one Pro user label Dec 5, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Dec 11, 2021

I love this feedback on the survey:

Why did you choose the free Data Grid version?
Because it is free and luckily we don't require the features from the paid version. Otherwise I would work to add the features my self than paying.

It reinforces how exposing the event and the state in the MIT version so developers can rebuild our features if they wish. Considering the average hourly rate of a developer in the world, after one day of work, it's likely a lot more expensive for the organization to have the dev work on it, than purchase one license. While the dev says the above, I'm pretty confident his manager will ask him to stop right away 馃槅. As long as MUI builds a great community around the MIT version, I think that we are good. TL;DR, I think that us spending time on exposing more of the apiRef (currently nothing, so it's easy to do more, one method would already be more) is the right move.

@cherniavskii
Copy link
Member

Closed by #6388

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! discussion RFC Request For Comments v6.x
Projects
None yet
Development

No branches or pull requests

5 participants