Material-ui: Improve CONTRIBUTING.md

[Yzrsah]

If we can make it easier to contribute to Material-UI, then it could help the project with more contributions.

• There is no web page with reproducible steps for making a working dev setup.
• CONTRIBUTING.md has incomplete information for making a link-able dev setup, only making reference to tell you that you should use yarn link.
• The material-ui workspace does not have scripts to produce a link-able build.
• A new contributor must spend days debugging imports, following errors, writing build scripts, opening issues, etc.
• To get started

The script

This is not documented:

curl https://codeload.github.com/mui-org/material-ui/tar.gz/next | tar -xz --strip=2 material-ui-next/examples/create-react-app-next
pushd create-react-app-next
    git clone https://github.com/mui-org/material-ui.git --branch next --single-branch --depth 1
    pushd material-ui
        yarn install
        yarn workspace @material-ui/utils build
        pushd packages/material-ui-utils/build
            yarn link
        popd
        yarn workspace @material-ui/system build
        pushd packages/material-ui-system/build
            yarn link
        popd
        pushd packages/material-ui-styles
            yarn link @material-ui/utils
            yarn build
            pushd build
                yarn link
            popd
        popd
        pushd packages/material-ui
            yarn link @material-ui/system
            yarn link @material-ui/utils
            yarn build
            pushd build
                yarn link
            popd
        popd
    popd
    yarn link @material-ui/core
    yarn link @material-ui/styles
    yarn link @material-ui/system
    yarn link @material-ui/utils
    yarn install
    npx yarn-list-link
    yarn start

All commands complete successfully and the dev server starts up, and then fails.

• Is this script even correct?
• How would a contributor know if it's correct or not, if it's not documented anywhere?
• And if it's not automated in the workspace's "scripts":?
• Is every contributor supposed to figure out the link-ed build dependency order so that the builds don't use the wrong links?
• (The script failed)

Errors with relative imports outside of src/

./src/withRoot.js
Module not found: You attempted to import ../esm/CssBaseline/index.js which falls outside of the project src/ directory. Relative imports outside of src/ are not supported.

NextJs version errors with hooks invariant violation:

curl https://codeload.github.com/mui-org/material-ui/tar.gz/next | tar -xz --strip=2 material-ui-next/examples/nextjs-next
pushd nextjs-next
...

Note that pages will be compiled when you first load them.
{ Invariant Violation: Hooks can only be called inside the body of a function component. (https://fb.me/react-invalid-hook-call)
    at invariant (/Users/foo/Source/nextjs-next/material-ui/node_modules/react/cjs/react.development.js:88:15)
    at resolveDispatcher (/Users/foo/Source/nextjs-next/material-ui/node_modules/react/cjs/react.development.js:1436:28)
    at Object.useContext (/Users/foo/Source/nextjs-next/material-ui/node_modules/react/cjs/react.development.js:1441:20)
    at StylesProvider (/Users/foo/Source/nextjs-next/material-ui/packages/material-ui-styles/build/StylesProvider.js:56:37)
    at processChild (/Users/foo/Source/nextjs-next/node_modules/react-dom/cjs/react-dom-server.node.development.js:2871:14)
    at resolve (/Users/foo/Source/nextjs-next/node_modules/react-dom/cjs/react-dom-server.node.development.js:2795:5)
    at ReactDOMServerRenderer.render (/Users/foo/Source/nextjs-next/node_modules/react-dom/cjs/react-dom-server.node.development.js:3185:22)
    at ReactDOMServerRenderer.read (/Users/foo/Source/nextjs-next/node_modules/react-dom/cjs/react-dom-server.node.development.js:3144:29)
    at renderToString (/Users/foo/Source/nextjs-next/node_modules/react-dom/cjs/react-dom-server.node.development.js:3629:27)
    at render (/Users/foo/Source/nextjs-next/node_modules/next-server/dist/server/render.js:42:16)
    at Object.renderPage (/Users/foo/Source/nextjs-next/node_modules/next-server/dist/server/render.js:102:16)
    at Function.module.exports../pages/_document.js.MyDocument.getInitialProps (/Users/foo/Source/nextjs-next/.next/server/static/development/pages/_document.js:2256
:18)
    at Object.loadGetInitialProps (/Users/foo/Source/nextjs-next/node_modules/next-server/dist/lib/utils.js:45:35)
    at Object.renderToHTML (/Users/foo/Source/nextjs-next/node_modules/next-server/dist/server/render.js:106:36) name: 'Invariant Violation', framesToPop: 1 }

This can be improved

• Can we make a testable, standardized script that will produce a working link-able local dev setup for Material-UI?

[eps1lon]

There is no web page with reproducible steps for making a working dev setup.

You mean testing changes to this package in your app? That's be nice I agree. The whole build infrastructure would do good with an overhaul

Is every contributor supposed to figure out the link-ed build dependency order so that the builds don't use the wrong links?

Of course not. However contributions should primarily be tested in this package. We will not accept changes that were only tested in your code. Personally I didn't have issues with linking packages.

How would a contributor know if it's correct or not, if it's not documented anywhere?

We have a section about this: https://github.com/mui-org/material-ui/blob/next/CONTRIBUTING.md#how-do-i-use-my-local-distribution-of-material-ui-in-any-project
Did you encounter issues with these steps? I'm not familiar with pushd so I can't really help you with the migration of the commands we described.

[Yzrsah]

We have a section about this: https://github.com/mui-org/material-ui/blob/next/CONTRIBUTING.md#how-do-i-use-my-local-distribution-of-material-ui-in-any-project
Did you encounter issues with these steps? I'm not familiar with pushd so I can't really help you with the migration of the commands we described.

Yes I did follow those steps from that page - but I had to add extra steps to resolve linkage across dependencies.

The pushd foo and popd is just like cd foo and cd .. in this case

[eps1lon]

Yes I did follow those steps from that page - but I had to add extra steps.

Well if our steps are working and your steps not and I don't know how your dev server works how am I supposed to help you?

[Yzrsah]

The steps listed in CONTRIBUTING.md produce this error when you use yarn link @material-ui/core in the create-react-app-next example:

curl https://codeload.github.com/mui-org/material-ui/tar.gz/next | tar -xz --strip=2 material-ui-next/examples/create-react-app-next
cd create-react-app-next
    git clone https://github.com/mui-org/material-ui.git --branch next --single-branch --depth 1
    cd material-ui
        yarn install
        cd packages/material-ui
            yarn build
            cd build
                yarn link
            cd ..
        cd ../..
    cd ..
    yarn link @material-ui/core
    yarn install
    yarn start

Failed to compile.

./src/withRoot.js
Module not found: You attempted to import ../esm/CssBaseline/index.js which falls outside of the project src/ directory. Relative imports outside of src/ are not supported.

And in the nextjs-next example:

curl https://codeload.github.com/mui-org/material-ui/tar.gz/next | tar -xz --strip=2 material-ui-next/examples/nextjs-next
cd nextjs-next
    git clone https://github.com/mui-org/material-ui.git --branch next --single-branch --depth 1
    cd material-ui
        yarn install
        cd packages/material-ui
            yarn build
            cd build
                yarn link
            cd ..
        cd ../..
    cd ..
    yarn link @material-ui/core
    yarn install
    yarn dev

Failed

{ Invariant Violation: Hooks can only be called inside the body of a function component. (https://fb.me/react-invalid-hook-call)

Tested on macOS and Linux
node v11.10.1

[Yzrsah]

@eps1lon

Yes I did follow those steps from that page - but I had to add extra steps.

Well if our steps are working and your steps not

My steps are your steps. You said to link, so I followed those instructions and then copy+pasted the resulting commands.

It links core, styles, system, and utils from branch next.

The steps implied in CONTRIBUTING.md are working only for older versions of Material-UI, not using Next branch with Next Examples on a HEAD dev setup

Did anyone try the scripts?

I don't know how your dev server works how am I supposed to help you?

The reason the scripts use curl to download examples and create a fresh server is so that you could see a reproduction without a custom setup, only relying on official examples from the material-ui repo, with Next versions.

[FMcIntosh]

I've had a couple of issues myself trying to set up a local distribution, with some similar error messages. I asked for help on Spectrum, so once this is resolved I'd be happy to help try and improve the Contributing guidelines if needed.

[eps1lon]

Looked into this a bit.

Best course of action right now is to change resolve.module in the webpack.config. For create-react-app users that have ejected this means:

-modules: ['node_modules'].concat(
+modules: [path.resolve('node_modules'), 'node_modules'].concat(

Without ejecting there's currently no workaround until facebook/create-react-app#6207 has landed.

This should fix the following issues:

• Module not found: You attempted to import ../esm/* which falls outside of the project src/ directory. Relative imports outside of src/ are not supported.
• SyntaxError .... <StylesProvider ...

Another solution would be to set resolve.symlink to false but changes to the linked packages will be ignored in watchmode. I think watchmode for symlinks is planned for webpack 5.

I would need a reproducible example to investigate the hooks error in nextjs. The error is actually way more generic than the message would let you believe.

In addition I've indentified the following contributor features:

• script that rebuilds all packages with a watchmode option
• link all built packages (using yarn link)
• documentation of that workflow

We appreciate any help offered with these items.

[FMcIntosh]

I can confirm that changing resolve.modules as suggested works for me. Thanks @eps1lon.

[pacurtin]

I'm a would-be first time contributor trying to set up my dev env and I'm running into these same problems. SyntaxError .... <StylesProvider ... to be specific. I'm lucky I stumbled upon this issue since facebook/create-react-app#6207 still hasn't been merged.

Maybe it would be a good idea to add the info here to CONTRIBUTING.md?

The approach I'm attempting is:

1. Use create-react-app to make a fresh react project
2. Build packages/material-ui
3. Use yarn link to import my local version into the fresh project
4. yarn start the fresh project
5. Make changes to packages/material-ui and rebuild it
6. Refreshing my fresh project in the browser should pick up the changes I made in packages/material-ui

Is this the recommended way to make a local dev setup?

[mbrookes]

Is this the recommended way to make a local dev setup?

Not really, you shouldn't need to make a fresh CRA site – you can test it with the docs site:

1. Fork material-ui on GitHub
2. Clone your fork
3. cd material-ui
4. yarn && yarn start
5. open http://localhost:3000

That's it! (Other than to optionally add the source repo as an upstream remote, in order to fetch updates)

[oliviertassinari]

Should we update the CONTRIBUTING.md guide to remove any mention/discourage usage of yarn link?

I start to think that testing the changes in another project is a pattern we should avoid. The maintainers should be able to review the pull requests, the maintainers don't have access to people projects. By removing the mention to yarn link, I would hope that we encourage the creation of reproduction examples and new demos.

[eps1lon]

Should we update the CONTRIBUTING.md guide to remove any mention/discourage usage of yarn link?

Sounds good to me. As far as I know yarn v2 will resolve linking issues and duplicate instances of react but we can always reintroduce mention of yarn lint once it becomes stable.

[croraf]

My opinion is that yarn link feature is a must-have!

Although making a test in your sandbox is surely needed prior to PR.

If it is not possible to fix yarn link immediately then I'm for removal of the section from CONTRIBUTING.md. Or even better to replace the section's text with a note like:

It is not possible to use yarn link at the moment, but will be in the future.

[eps1lon]

My opinion is that yarn link feature is a must-have!

In my experience this leads to PRs that have neither documentation nor tests updated. By not enabling yarn link we force contributors to write a test or change the documentation which means less friction when reviewing the PR. The contributor knows that this is working because they had some sandbox locally but the maintainer doesn't know that and can't maintain the current solution since they can't know what this implementation should actually achieve.

[croraf]

I suggest we rework the CONTRIBUTING.md page. I think it should start like this:
https://gist.github.com/croraf/c8255bd5d5eb5adca4760cdc80b576a4

any notes and guidelines will be below these sections.
What do you think?

[croraf]

This is what I have atm: https://gist.github.com/croraf/c8255bd5d5eb5adca4760cdc80b576a4

This is what still has to be filtered and included in the document: https://gist.github.com/croraf/9735cb863321aa7d6c0e89b4bdb22e2c

[oliviertassinari]

I think that we close, we should have addressed the concerns, we have removed the linkable section. Let's wait to get feedback from the new contributing guide.