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

[core] Remove use of getState #2300

Merged
merged 4 commits into from
Aug 12, 2021
Merged

Conversation

oliviertassinari
Copy link
Member

A follow-up on #2208 (comment). I don't get why we even have such getState method. For #820, I think that we could restructure the state, and maybe have getState returns a subset of the state that is public.

@oliviertassinari oliviertassinari added the core Infrastructure work going on behind the scenes label Aug 7, 2021
@oliviertassinari oliviertassinari changed the title [core] Remvoe usage of getState [core] Remove usage of getState Aug 7, 2021
@flaviendelangle
Copy link
Member

@m2mathew do you remember exactly what was the purpose of .getState() and if it makes sense to use it in our codebase ?
I agree that it's lighter to just access the property, but I may be missing something here.

@oliviertassinari
Copy link
Member Author

We had a getState(subpart: string) API at some point 🤷‍♂️, happy with whatever direction we take here.

Copy link
Member

@m4theushw m4theushw left a comment

Choose a reason for hiding this comment

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

We already removed in #2141 the ability to return a subset of the state because it was never used. I'm OK with removing this method or making it private.

@@ -8,8 +8,7 @@ export interface GridStateApi {
state: GridState;
/**
* Returns the state of the grid.
* @param {string} stateId The part of the state to be returned.
* @returns {any} The state of the grid.
* @returns {GridState} The state of the grid.
Copy link
Member

Choose a reason for hiding this comment

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

Should we not document it for now?

Suggested change
* @returns {GridState} The state of the grid.
* @returns {GridState} The state of the grid.
* @ignore - do not document.

Copy link
Member

Choose a reason for hiding this comment

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

If we stop using it internally, we can just remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

@flaviendelangle Removing the method would be a breaking change. We can move more iteratively.

@m4theushw
Copy link
Member

m4theushw commented Aug 11, 2021

I've resolved the conflicts, removed a few remaining getState and made it private. I think we can merge it.

@mbrookes mbrookes changed the title [core] Remove usage of getState [core] Remove use of getState Aug 11, 2021
@m4theushw m4theushw merged commit c682f5f into mui:master Aug 12, 2021
@oliviertassinari oliviertassinari deleted the normalize-state branch August 12, 2021 14:54
@oliviertassinari
Copy link
Member Author

@m4theushw Thanks

@oliviertassinari oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Aug 12, 2021
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! core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants