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

[Hidden] Remove dependency on hoist-non-react-statics #33015

Merged

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Jun 4, 2022

The API was removed in #26136 but we forgot to remove this dead logic. I found it thanks to https://npm.anvaka.com/#/view/2d/%2540mui%252Fmaterial, one of the few licenses that is not MIT:

Screenshot 2022-06-04 at 16 30 05

I think that all-in on MIT makes it easier for adoption. Fewer licenses to get approval on in corporations.

@oliviertassinari oliviertassinari added performance core Infrastructure work going on behind the scenes labels Jun 4, 2022
@oliviertassinari oliviertassinari marked this pull request as ready for review June 4, 2022 14:31
@mui-bot
Copy link

mui-bot commented Jun 4, 2022

Details of bundle changes

Generated by 🚫 dangerJS against 85d3958

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 4, 2022

Off-topic. Our bundle size tracker looks broken. Why is the change not reported in the @mui-bot's comment?

Screenshot 2022-06-04 at 17 15 05

https://mui-dashboard.netlify.app/size-comparison?circleCIBuildNumber=393086&baseRef=master&baseCommit=2f962591eb819c6a105d29e590c056bffffc3616&prNumber=33015

If the same problem also happens when the bundle size increases, then it feels critical.


I also think that it starts to be important for MUI X to have this mui/mui-x#5550, so we can start to optimize bundle size for the data grid and date picker. The data grid is as big as Material UI: https://bundlephobia.com/package/@mui/x-data-grid@5.12.0

@oliviertassinari oliviertassinari added the component: Hidden The React component. label Jun 4, 2022
@oliviertassinari oliviertassinari changed the title [core] Remove dependency on hoist-non-react-statics [Hidden] Remove dependency on hoist-non-react-statics Jun 4, 2022
@oliviertassinari oliviertassinari removed the core Infrastructure work going on behind the scenes label Jun 4, 2022
@oliviertassinari oliviertassinari requested a review from a team June 5, 2022 10:01
@@ -108,8 +107,6 @@ const withWidth =
WithWidth.displayName = `WithWidth(${getDisplayName(Component)})`;
}

hoistNonReactStatics(WithWidth, Component);
Copy link
Member Author

Choose a reason for hiding this comment

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

For context, Component can only be Hidden internal components, so this line is useless.

Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

👍 The fewer dependencies, the better!

@mnajdova mnajdova merged commit 50286f1 into mui:master Jun 13, 2022
@oliviertassinari oliviertassinari deleted the remove-hoist-non-react-statics branch June 13, 2022 12:41
@oliviertassinari
Copy link
Member Author

The fewer dependencies, the better!

Haha, yes 😍. AG Grid pushes it too far though https://www.npmjs.com/package/ag-grid-community no? 😅

Screenshot 2022-06-13 at 14 48 18


Oh, the non-MIT license was not ISC but BSD in the Core. ISC is the weird license that MUI X Pro depends on (that we are phasing out).

@michaldudak
Copy link
Member

AG Grid pushes it too far though https://www.npmjs.com/package/ag-grid-community no? 😅

Definitely :)

Oh, the non-MIT license was not ISC but BSD in the Core

AFAIK both BSD and ISC are OK to use in our context (ISC being even more permissive): https://tldrlegal.com/license/-isc-license, https://tldrlegal.com/license/bsd-3-clause-license-(revised)

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Jun 14, 2022

AFAIK both BSD and ISC are OK to use in our context

@michaldudak Agree, the only downside is that it's one more license that developers have to check (what you did). Now, it's only a minor detail.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants