-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[charts] Add Initializable type and behaviour to allow checking if a complex context has been initialized. #13365
Conversation
Deploy preview: https://deploy-preview-13365--material-ui-x.netlify.app/ |
export function useSeries() { | ||
const series = React.useContext(SeriesContext); | ||
export function useSeries(): FormattedSeries { | ||
const { isInitialized, ...series } = React.useContext(SeriesContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how much it applies here, but in the grid I've been removing a lot of spread operator usages due to their cost. If there is a way to avoid it here, it might be nice to do it. E.g. split the boolean flag and the data as { isInitialized: boolean, data: T }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see reason of not doing this split between the data
and the isInitialized
. Since only hooks are user-facing, we can add the data
in context and return the data
when using the hook.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had it ready, I just spent a lot of time trying to understand why the karma tests are failing when using the ErrorBoundary
. It seems it is an issue with our karma setup here: https://github.com/mui/material-ui/blob/next/packages-internal/test-utils/src/setupKarma.js
and also possibly that we override the console
somewhere I couldn't find. But in the end I couldn't find a proper solution to it, so I just skip it in karma 😓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the input btw
Outside of the romgrk comments, that looks nice 👍 |
…complex context has been initialized. (mui#13365)
…complex context has been initialized. (mui#13365)
Initializable<T>
typeuseSvgRef
)useContext
Reference
Contexts are never
undefined
, they always have their default values, so the previous checks were doing nothing.For contexts where the type is allow
null|undefined
but is required to be non-null at runtime, we can just check for thuthness