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

[charts] Add Initializable type and behaviour to allow checking if a complex context has been initialized. #13365

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

JCQuintas
Copy link
Member

  • Add Initializable<T> type
  • Make sure complex user-facing contexts are initializable so we can more consistently check if they are actually present in the scope.
  • Make sure simple user-facing contexts are initializable (useSvgRef)
  • Add tests to ensure the checks
  • Use hooks in place of 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

@JCQuintas JCQuintas added enhancement This is not a bug, nor a new feature component: charts This is the name of the generic UI component, not the React module! labels Jun 4, 2024
@JCQuintas JCQuintas self-assigned this Jun 4, 2024
@mui-bot
Copy link

mui-bot commented Jun 4, 2024

Deploy preview: https://deploy-preview-13365--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 78a904f

export function useSeries() {
const series = React.useContext(SeriesContext);
export function useSeries(): FormattedSeries {
const { isInitialized, ...series } = React.useContext(SeriesContext);
Copy link
Contributor

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 }.

Copy link
Member

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.

Copy link
Member Author

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 😓

Copy link
Member Author

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

@alexfauquette
Copy link
Member

Outside of the romgrk comments, that looks nice 👍

@JCQuintas JCQuintas requested a review from romgrk June 5, 2024 10:05
@JCQuintas JCQuintas merged commit 8f0778b into mui:master Jun 5, 2024
17 checks passed
@JCQuintas JCQuintas deleted the fix-context-check branch June 5, 2024 10:23
DungTiger pushed a commit to DungTiger/mui-x that referenced this pull request Jul 23, 2024
thomasmoon pushed a commit to thomasmoon/mui-x that referenced this pull request Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants