-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Update contributing.md #16508
Update contributing.md #16508
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also completely remove TEST_ONLY
/TEST_GREP
from our scripts?
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/56944 |
Personally I don't use them either. @liuxingbaoyu Do you think they should be kept? |
Thanks for mentioning! I didn't use them. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
```sh | ||
yarn run --inspect-brk jest -i babel-plugin-arrow-functions -t "destructuring parameters" | ||
``` | ||
|
||
If you plan to stay long in the debugger (which you'll likely do!), you may increase the test timeout by the [Jest CLI `--testTimeout` option](https://jestjs.io/docs/cli#--testtimeoutnumber). For example to increase test timeout to 500 seconds | ||
|
||
```sh | ||
yarn run --inspect-brk jest -i babel-parser -t "my new test" --testTimeout=500000 | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great example! Maybe --runInBand
is useful here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well -i
is actually an alias for --runInBand
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry missed it. :)
|
||
_Yarn_: Make sure that Yarn 1 is [installed](https://classic.yarnpkg.com/en/docs/install) with version >= `1.19.0`. | ||
_Yarn_: Make sure that Yarn is [installed](https://yarnpkg.com/getting-started/install) with version `>=4.0.0`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be no actual installation of yarn v4 here, unfortunately the yarn official website only leaves the corepack tutorial.
Let's change the wording to something like configure to support yarn v4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well upon the first timing running corepack enable yarn
, corepack will download the latest yarn binary and "install" it anyway. Contributors should refer to the official yarn docs for how to install yarn, hence the updated link.
Therefore I think the current wording is fine, if we change it to something like "configure corepack to install yarn 4" but the yarn community one day decides to change their install recommendation, we will have to sync with them. For the time being, simply put "installed" seems sufficient.
In this PR we update the project's build requirements and promote the usage of
yarn jest
. Compared tomake test-only
, contributors can refer to Jest's docs for more cli options.