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] Remove stateId argument from GridApi getState method #2141

Merged
merged 11 commits into from
Jul 21, 2021

Conversation

flaviendelangle
Copy link
Member

@flaviendelangle flaviendelangle commented Jul 13, 2021

@dtassone, we only have 2 calls of getState with a stateId, both in the same hook (useGridFilter)
Is this something we want to keep ?
95% of the time, the code written is const page = apiRef.current.getState().page instead of const page = apiRef.current.getState('page') so I'm not sure the overhead is worth it here.

But with this PR you can use the stateId with the correct typing without specifying it as a generic.

@m4theushw the doc generation does not like the generic, I'm loosing the StateId definition here. Same thing for the type returned.
It could be interesting to handle it. But I would more be in favor of droping the stateId param on getState (see above)

@flaviendelangle flaviendelangle requested a review from dtassone July 13, 2021 13:31
@flaviendelangle flaviendelangle changed the title [core] Add type inferance in apiRef.current.getState based on key passed [core] Add type inferance in apiRef.current.getState based on stateId Jul 13, 2021
@flaviendelangle flaviendelangle self-assigned this Jul 13, 2021
@flaviendelangle flaviendelangle added core Infrastructure work going on behind the scenes typescript labels Jul 13, 2021
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.

95% of the time, the code written is

Then +1 for removing these 5% percent. For instance, it's not supported in https://react-redux.js.org/api/hooks#usestore.

-const filterState = apiRef.current.getState('filter');
+const filterState = apiRef.current.getState().filter;

or even:

-const filterState = apiRef.current.getState('filter');
+const filterState = filterGridStateSelector(apiRef.current.getState());

@flaviendelangle
Copy link
Member Author

flaviendelangle commented Jul 19, 2021

In general, I'm in favor of avoiding methods with separate behaviors like this one.
If the stateId was something usefull, I would have prefered splitting the two :

getState: () => GridState
getSubState: <StateId extends keyof GridState>(stateId: StateId) => GridState[StateId]

@flaviendelangle flaviendelangle changed the title [core] Add type inferance in apiRef.current.getState based on stateId [core] Remove stateId argument from apiRef getState method Jul 19, 2021
@flaviendelangle flaviendelangle changed the title [core] Remove stateId argument from apiRef getState method [core] Remove stateId argument from GridApi getState method Jul 19, 2021
@oliviertassinari
Copy link
Member

The API is public, I have added a new label. If we could update the description of the PR to document it, it would be perfect. Actually, are we confortable with making the whole state public? If we are not, we can make the method private.

@oliviertassinari oliviertassinari changed the title [core] Remove stateId argument from GridApi getState method [DataGrid] Remove stateId argument from GridApi getState method Jul 19, 2021
@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jul 19, 2021
@m4theushw
Copy link
Member

Actually, are we confortable with making the whole state public? If we are not, we can make the method private.

@oliviertassinari Yes, I would keep the state and this method public. For some parts of the state (e.g. focus) there's no alternative way to access them besides using getState.

@flaviendelangle
Copy link
Member Author

For the stable version, we may want to make it private and only expose methods for the wanted substates.
That would allow us to consider the other sub states as internal and change their structure without any breaking change.

But for now I do agree that exposing it is simpler.

@oliviertassinari
Copy link
Member

OK, I guess that once the component is stable and we need to change one part of the state, then we can change the public getState method to keep it unchanged for developers.

@oliviertassinari oliviertassinari removed their request for review July 20, 2021 13:17
@flaviendelangle flaviendelangle merged commit ee36b05 into mui:master Jul 21, 2021
@flaviendelangle flaviendelangle deleted the typing-getState branch July 21, 2021 07:38
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! core Infrastructure work going on behind the scenes typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants