hupe1980 / gatsby-plugin-material-ui

Gatsby plugin for Material-UI with built-in server-side rendering support
MIT License
136 stars 24 forks source link

Maybe the Install Instruction should include `material-ui/core`? #40

Closed FeynmanDNA closed 5 years ago

FeynmanDNA commented 5 years ago

Currently the install instruction is as followed: Install npm install gatsby-plugin-material-ui @material-ui/styles

However, in order for the components from material-ui to work, we still need to install @material-ui/core.

I am wondering why the installation is only installing material-ui/styles?

FeynmanDNA commented 5 years ago

In fact, the material-ui example using this plugin did not list material-ui/styles as a dependency: https://github.com/mui-org/material-ui/blob/master/examples/gatsby/package.json:

  "dependencies": {
    "@material-ui/core": "latest",
    "gatsby": "latest",
    "gatsby-plugin-material-ui": "latest",
    "gatsby-plugin-react-helmet": "latest",
    "react": "latest",
    "react-dom": "latest",
    "react-helmet": "latest"
  },
oliviertassinari commented 5 years ago

@FeynmanDNA Thanks for raising this concern. I think that this plugin should have the styles package as a peer dependency, as the styled-components plugin does https://github.com/gatsbyjs/gatsby/blob/af842dde82860be0fdb6a8d01df3cac202d5deaf/packages/gatsby-plugin-styled-components/package.json#L29

The example on Material-UI relies on the transitive installed styles dependency (form core).

We have a related discussion on this topic in https://github.com/mui-org/material-ui/issues/17387#issuecomment-530544534

I don't think that we should change anything. This plugin focus on having the styles work, we might no longer need it once we migrate to styled-components.

Did you have a look at the gatsby material-ui theme package? We could consider updating the Material-UI Gatsby example to use it directly. It would remove some freedom, but that might not be needed, to investigate. At least, I think that we should link it so people can better discover it. cc @hupe1980

FeynmanDNA commented 5 years ago

@oliviertassinari Thank you for your helpful links in the answer.

Regarding the Material-UI example, for the gatsby-plugin-material-ui, we are indeed using @material-ui/styles for the ThemeProvider in the TopLayout.js file: https://github.com/mui-org/material-ui/blob/master/examples/gatsby/plugins/gatsby-plugin-top-layout/TopLayout.js#L5

However, for the package.json, @material-ui/styles is not mentioned at all as any form of dependency.

So how does the Material-UI example know where to get the @material-ui/styles?

FeynmanDNA commented 5 years ago

@oliviertassinari I just look through the package-lock.json, and it looks like installing @material-ui/core will automatically install @material-ui/styles?

    "@material-ui/core": {
      "version": "4.4.2",
      "resolved": "https://registry.npmjs.org/@material-ui/core/-/core-4.4.2.tgz",
      "integrity": "sha512-hnZ4SP/hWJ9sUoNMkStz/y/CL2c7j4JpVIB2py3+vpBFU9TgHL3noBk3Fr0gltRvvlYA9ekpiGsGZ2ukk1R7Eg==",
      "requires": {
        "@babel/runtime": "^7.4.4",
        "@material-ui/styles": "^4.4.1",
oliviertassinari commented 5 years ago

Yes, it's what I meant with "transitive".

hupe1980 commented 5 years ago

Did you have a look at the gatsby material-ui theme package? We could consider updating the Material-UI Gatsby example to use it directly. It would remove some freedom, but that might not be needed, to investigate. At least, I think that we should link it so people can better discover it. cc @hupe1980

@oliviertassinari I can make a PR for an example with gatsby-theme-material-ui

oliviertassinari commented 5 years ago

@hupe1980 Good for me, we could name it/examples/gatsby-themes/? I think that it's interesting enough to have people discover it. We need to explain the difference with /examples/gatsby/. For instance, we could say that the new version of the gatsby example abstracts all the modules. It's less boilerplate / freedom in exchange for a "It just work" outcome. I'm curious to see the other core contributors feedback on the changes.