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

[docs] Remove unneeded dependencies from examples #9746

Merged

Conversation

cherniavskii
Copy link
Member

This PR is continuation of #9735 and is raised by #9732.

Note, that examples/create-react-app-with-jss example will not work until beta.27 is released, since jssPreset is not exposed in earlier versions.
So it's better to not merge it until beta.27 is released.

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.

We need some of the dependencies. Otherwise, it's going in the right direction :)

@@ -3,13 +3,10 @@
"version": "1.0.0",
"private": true,
"dependencies": {
"jss": "latest",
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

But jss is listed in material-ui dependencies, which means that it is installed along with material-ui and can be imported from node_modules in any place within application

Copy link
Member

Choose a reason for hiding this comment

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

It's a transitive dependency, it's brittle.

"material-ui": "next",
"prop-types": "latest",
"react": "latest",
"react-dom": "latest",
"react-jss": "latest",
Copy link
Member

Choose a reason for hiding this comment

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

@@ -3,13 +3,11 @@
"version": "1.0.0",
"private": true,
"dependencies": {
"jss": "latest",
Copy link
Member

Choose a reason for hiding this comment

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

"material-ui": "next",
"next": "latest",
"prop-types": "latest",
"react": "latest",
"react-dom": "latest",
"react-jss": "latest"
Copy link
Member

Choose a reason for hiding this comment

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

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Jan 5, 2018
@oliviertassinari
Copy link
Member

Wait, hold on. I think that we should add JSS as peer dependency to Material-UI to mitigate the bundle size and class name counter duplication issues. What do you think?

@cherniavskii
Copy link
Member Author

@oliviertassinari

I think that we should add JSS as peer dependency

I think keeping jss as material-ui dependency is better. It will be installed with material-ui, user doesn't have to worry about installing other deps.

to mitigate the bundle size

I believe moving it to peerDependencies would not have any impact on app's bundle size.

Maybe @kof can help us to decide?

@kof
Copy link
Contributor

kof commented Jan 5, 2018

If jss would be something user has to setup, I would go for peerDependency, but I think in case of mui, jss is an internal implementation detail and mui should keep it as far as possible from the user, unless user needs to customize it.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 5, 2018

capture_d ecran_2018-01-04_a_14 52 56
(material-ui is v1.0.0-beta.17 while material-ui-next is v1.0.0-beta.26 in this screenshot)

Here is the issue I'm facing at the office. Our customer-facing components are relying on an up to date version of Material-UI, while our admin backend (admin-on-reset is relying on an older version). Because of material-ui-icons importing material-ui, we have a duplicated bundle issue. I can easily imagine the same configuration with the Material-UI package extensions available out there.
Duplicated modules are something that happens often in a bundle. However, it starts to be an issue when it means bundling twice a large CSS-in-JSS runtime.

I see the following solutions:

  • I share the same Material-UI versions, but I don't want that. I don't want the admin to block user improvements.
  • 👍 having JSS as a peer dependency. 20% effort for 80% of the outcome.
  • We split our app monolith into two micro-services and package.json instead of one.
  • having withStyles and the transitive dependencies as a peer dependency. Effectively, it's a styling solution on his own. It's more stable than our components' API. It can have his own lifecycle. We used to have something close with @nathanmarks solution: https://github.com/nathanmarks/jss-theme-reactor.

@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Jan 6, 2018
@oliviertassinari oliviertassinari dismissed their stale review January 6, 2018 10:41

I have added the transitive dependencies back

@cherniavskii
Copy link
Member Author

Because of material-ui-icons importing material-ui, we have a duplicated bundle issue.

Looks like that issue is caused by non-working tree shaking

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 6, 2018

I'm not expecting the dependency to be tree shaked. I'm importing material-ui-icons that is importing material-ui that is importing another version of jss.

v1.0.0-beta.26
https://github.com/mui-org/material-ui/blob/bd42549ee464bce03743f7ee18964066f9d3907d/package.json#L71
v1.0.0-beta.17
https://github.com/mui-org/material-ui/blob/616d69d5ff7263eb9c84960b6b7a4c9ff852d4ef/package.json#L69

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 6, 2018

After all, having JSS as a peer dependency will no help. It will only prevent the bundle size duplication, but it's actually less freedom for our users. For instance, in my case. I'm willing to accept the bundle size duplication in the short term so I can ship my feature.

@oliviertassinari
Copy link
Member

One take away: it's important to keep the scope of Material-UI's dependencies as wide as possible.

@oliviertassinari oliviertassinari merged commit c3948a8 into mui:v1-beta Jan 6, 2018
@oliviertassinari
Copy link
Member

v1.0.0-beta.27 has been released, we can update the examples now. I'm merging. @cherniavskii Thank you!

@cherniavskii
Copy link
Member Author

@oliviertassinari did you consider bundle splitting in your specific case? One bundle for user and other one for admin? And maybe one more bundle with core dependencies?
I heard it's possible in webpack and that's a real boost for users - they don't have to download the code they'd never use

@cherniavskii cherniavskii deleted the update-examples-dependencies branch January 6, 2018 20:30
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 6, 2018

did you consider bundle splitting in your specific case? One bundle for user and other one for admin? And maybe one more bundle with core dependencies?

@cherniavskii We use Next.js, we have around 30 chunk files. Given how Next.js tell webpack to build the share core bundle, you are right. We would be better off with 2 different Next.js app, one for the customers and one for the admin 👍 .

@cherniavskii
Copy link
Member Author

@oliviertassinari I should definitely check out Next.js and it's features!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants