Improve CONTRIBUTING.md #14760

Yzrsah · 2019-03-06T08:46:18Z

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?

The text was updated successfully, but these errors were encountered:

eps1lon · 2019-03-06T09:14:56Z

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 · 2019-03-06T09:25:48Z

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 · 2019-03-06T09:26:06Z

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 · 2019-03-06T10:33:28Z

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 · 2019-03-07T08:22:39Z

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

fraserdrops · 2019-03-19T21:48:33Z

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 · 2019-03-20T15:12:18Z

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.

fraserdrops · 2019-03-21T10:14:07Z

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

pacurtin · 2019-09-09T21:42:13Z

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:

Use create-react-app to make a fresh react project
Build packages/material-ui
Use yarn link to import my local version into the fresh project
yarn start the fresh project
Make changes to packages/material-ui and rebuild it
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 · 2019-09-09T22:42:55Z

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:

Fork material-ui on GitHub
Clone your fork
cd material-ui
yarn && yarn start
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 · 2019-09-14T13:52:20Z

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 · 2019-09-14T14:04:38Z

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 · 2019-09-15T09:50:10Z

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 · 2019-09-15T11:20:20Z

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 · 2019-09-16T10:43:52Z

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 · 2019-09-19T17:48:22Z

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 · 2019-10-06T11:08:52Z

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.

eps1lon added the docs Improvements or additions to the documentation label Mar 6, 2019

oliviertassinari mentioned this issue Jul 15, 2019

Cannot use linked lib using material-ui #16606

Closed

2 tasks

oliviertassinari changed the title ~~It's Excessively Difficult to get started becoming a contributor to Material-UI~~ Improve CONTRIBUTING.md Sep 16, 2019

This was referenced Sep 21, 2019

[docs] CONTRIBUTING.md reword branch structure, remove Build, Yarn Link #17501

Merged

[docs] Improve Contributing.md - part 2 #17562

Closed

oliviertassinari closed this as completed Oct 6, 2019

chapati23 mentioned this issue Mar 10, 2021

[react-scripts] Fix module resolution for yarn linked components facebook/create-react-app#6207

Open

Uh oh!

Improve CONTRIBUTING.md #14760

Improve CONTRIBUTING.md #14760

Comments

Yzrsah commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The script

Errors with relative imports outside of src/

NextJs version errors with hooks invariant violation:

This can be improved

eps1lon commented Mar 6, 2019

Uh oh!

Yzrsah commented Mar 6, 2019

Uh oh!

eps1lon commented Mar 6, 2019

Uh oh!

Yzrsah commented Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Yzrsah commented Mar 7, 2019

Uh oh!

fraserdrops commented Mar 19, 2019

Uh oh!

eps1lon commented Mar 20, 2019

Uh oh!

fraserdrops commented Mar 21, 2019

Uh oh!

pacurtin commented Sep 9, 2019

Uh oh!

mbrookes commented Sep 9, 2019

Uh oh!

oliviertassinari commented Sep 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eps1lon commented Sep 14, 2019

Uh oh!

croraf commented Sep 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eps1lon commented Sep 15, 2019

Uh oh!

croraf commented Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

croraf commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviertassinari commented Oct 6, 2019

Uh oh!

Yzrsah commented Mar 6, 2019 •

edited

Loading

Yzrsah commented Mar 6, 2019 •

edited

Loading

oliviertassinari commented Sep 14, 2019 •

edited

Loading

croraf commented Sep 15, 2019 •

edited

Loading

croraf commented Sep 16, 2019 •

edited

Loading

croraf commented Sep 19, 2019 •

edited

Loading