-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
Release script #11223
Release script #11223
Conversation
Tagging a few peeps who might be interested in reviewing this. Happy to chat about ideas for improvements or concerns. |
• Make sure all contributors are credited. | ||
• Verify that the markup is valid by previewing it in the editor: {blue.bold ${CHANGELOG_PATH}} | ||
|
||
{bold.underline Step 2: Smoke test the packages} |
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.
Could this be automated using WebDriver? Definitely a separate thing, but might be worth considering adding to the script in the future.
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.
Maaaybe but I am not a very big fan of WebDriver automated tests. 😄
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.
I think @nhunzaker is looking at this.
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.
We can always follow up with that. I'm reluctant about introducing WebDriver into our release process. It's always a pain in the ass. But I won't be stubborn about it if others feel differently.
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.
I agree. I would hate for a webdriver test to block a release. I think we'd be more successful sending out edge releases so that issue filers can quickly verify their fixes.
I hadn't thought of using webdriver for the packages, only specifically for the manual DOM test fixtures.
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.
Good news is, the new release script should make preview releases easy too!
./scripts/release/build.js -v 16.1.0-rc.0
# Do manual fixture verification...
# Or run automated WebDriver tests if they exists
./scripts/release/publish.js -v 16.1.0-rc.0
# 16.1.0-rc.0 will be published to NPM with @next tag
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.
I also like the idea of automated tests for the fixtures stuff- but I would prefer not to put it into the release script. The release script could make mention of the automated scripts though, just not block on them.
Sure, but at the moment this is essentially exactly like a Webdriver test except someone is manually running it :P
|
So it's significantly better than a WebDrive test. 😄 |
Fwiw, I think @ForbesLindesay wrote some tooling around browser tests which makes them much less flaky than WebDriver tests usually are. Might be worth looking into. |
Thanks for the note, Max. Let's follow up later with that bit. 😄 I'd like to get this script reviewed and merged in time to do the upcoming 16.1 release with it if possible. |
WebDriver can be pretty solid, particularly on a page specifically build for testing. It's less reliable for testing UI elements that change very frequently. Our internal WebDriver tests at Facebook are very solid now. Most of the flakiness came from environment configuration, which is something you also hit with similar manual tests. For example, internally a lot of the issues were around database deadlocks when trying to execute a large number of tests in parallel, and issues with slow JavaScript Babel builds (which also happens during production deploys, and the first time you hit a devserver). A lot of the issues weren't actually specific to the test environment. Configured properly, a WebDriver test shouldn't be any more flaky than a manual test that performs similar steps and uses a similarly-configured environment. The key is to have a solid environment. Containerization (eg. Docker) is great for that.
I agree that tests should be executed separately to a release script. 👍 For Yarn we do block releases on tests, but we try to keep the tests stable so that it's not an actual issue in production (if a test fails, it's likely that something is broken, and we don't want to release a broken build!)
Thank you, sounds good! |
@@ -1,24 +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.
I love the sound of code being deleted ^_^
scripts/release/README.md
Outdated
|
||
Run a script without any parameters to see its usage, eg: | ||
``` | ||
node ./build.js |
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 this have node
removed since build.js
has the #!/usr/bin/env node
hashbang?
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.
Yeah, it's optional, provided the file has the executable permission, which it will 😄
|
||
const invalidDependencies = []; | ||
|
||
const checkModule = async module => { |
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.
I probably don’t know async/await well enough, but why does this function need to be marked async
? It looks fully synchronous internally.
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.
Yeah, probably just a difference between how I thought I'd write the code and how I ended up writing it. You're right that async
could be removed from this function.
{ | ||
content: chalk | ||
.hex('#61dafb') | ||
.bold(figlet.textSync('react', {font: 'Graffiti'})), |
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.
so that’s how you’re generating the fun artwork!
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.
I actually generated it manually 😅 before remembering the figlet
module heh
2. It should say {italic "Hello world!"} | ||
3. Next go to {yellow.bold fixtures/packaging} and run {bold node build-all.js} | ||
4. Install the "serve" module ({bold npm install -g serve}) | ||
5. Go to the repo root and {bold run serve -s .} |
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.
where does run
come from? 🤔
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.
Should just be serve
. My bad.
After completing the above steps, resume the release process by running: | ||
{yellow.bold ${command}} | ||
`.replace(/\n +/g, '\n') | ||
); |
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.
should running yarn install && yarn test
inside fixtures/reconciler
happen in 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.
Want to take a stab at writing up some new bullets for this?
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.
want that before you land this PR or as a folow-up?
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.
Up to you I guess, but after would be nice.
const updateProjectPackage = async project => { | ||
const path = join(cwd, 'packages', project, 'package.json'); | ||
const json = await readJson(path); | ||
json.version = version; |
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.
I think react-reconciler
should be special cased here to not have lock-step versioning (though not sure what to do besides just give it minor bumps)
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.
Hm. Why would we want to release reconciler independently? In the past, we've released react-* things in lock step (even though often not strictly required) b'c it's easier for external people to see and reason about.
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.
🤔 Since the API isn’t semver stable yet keeping it sub 1.x avoids issues for breaking the likely breaking API changes that will take place as the async work makes progress.
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.
Gotcha.
I see 2 potential downsides for this change:
1: It adds complexity to the automated process we are trying to streamline. (I'm not saying this a compelling argument not to do it but it is a downside.)
We could require a separate argument for the reconciler version- or we could require the release engineer to manually update it before hand but both are a worse experience for the release engineer.
Maybe we should just handle the case of a version < 1 by auto-incrementing (eg 0.1.0
is less than 1.0.0
so we special case to auto-bump to 0.1.1
). What are your thoughts? Is it important to be able to choose between 0.2.0
and 0.1.1
in that case or will a minor version bump suffice for pre-release versioning?
2: I worry it adds confusion for end-users. It's a nicer developer experience to know that react-reconciler
version X is compatible with react
version X without having to rely on the (typically noisy, often overlooked) peer dependency warning mechanism.
I think this is more important and we should aim to synchronize the version of react-reconciler
with react
as soon as the API stabilizes.
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.
We definitely should not be versioning the reconciler package together with other packages right now. I'd be okay bumping a minor on every release (e.g. 0.1.0
-> 0.2.0
-> 0.3.0
). It is changing way too fast and I don't see it stabilizing for another six months or so.
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.
Releasing patch by default won't work too. We will break it between main React patches pretty often. Ideally we should increment a 0.x
minor for it when the Flow file changes, but I think the low-maintenance “always increment” approach will work fine in the meantime. People using this know it’s very experimental anyway.
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.
I worry it adds confusion for end-users
There are, like, 10 end users. Only people creating custom renderers are end users, and they are already used to patching React and digging up super complicated internals just to get it working. So this is already a large improvement for them.
t's a nicer developer experience to know that react-reconciler version X is compatible with react version X
There shouldn't be an issue with react
<-> react-reconciler
dependencies and we can safely say any react-reconciler
version is friends with react@16
. This is because we don’t expose “important” things from react
package anymore.
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.
We definitely should not be versioning the reconciler package together with other packages right now.
Yes, that's become apparent 😄 I didn't realize the degree of expected instability for this package because I haven't been following it closely.
but I think the low-maintenance “always increment” approach will work fine in the meantime.
Great.
There are, like, 10 end users.
That's a good thing to be reminded of.
There shouldn't be an issue with react <-> react-reconciler dependencies and we can safely say any react-reconciler version is friends with react@16. This is because we don’t expose “important” things from react package anymore.
I'm skeptical of this 😄 but so long as this is a temporary thing (unlocked versions) until the reconciler package stabilizes, I'm not opposed.
I really enjoyed reading this code! Breaking things into well named and delineated modules makes it easy to review, thanks! I left some non-blocking-nits and compliments, and I'm going to do a quick dry run as part of reviewing this. |
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.
Going to do a dry-run manual test before passing a verdict.
const checkProject = async project => { | ||
const owners = (await execRead(`npm owner ls ${project}`)) | ||
.split('\n') | ||
.filter(owner => owner) |
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.
I don't think this filter call is doing anything?
We can probably delete that line.
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.
Oh wait - is it stripping empty strings or something? Because of '' being falsy?
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.
It removes empty lines. For example:
str = `
a
b
`;
str.split('\n') // ["", "", "a", "", "b", "", ""]
str.split('\n').filter(owner => owner) // ["a", "b"]
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.
An idiomatic JS way of doing this that I've seen is .filter(Boolean)
, but maybe that's not clear to beginners? owner => owner
isn't clear either though. I'd suggest doing .filter(owner => !!owner)
if you want to go that way.
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.
I like .filter(Boolean)
well enough. 👍
I also think owner => owner
is fine and the unnecessary !!owner
cast nags me (even though it wouldn't actually matter in this case).
const {exec} = require('child-process-promise'); | ||
const {logPromise} = require('../utils'); | ||
|
||
const install = async ({cwd}) => await exec('yarn', {cwd}); |
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.
At first I thought maybe we could use yarn upgrade
here and combine with the later step, but it seems like that is different since it involves commiting change to the lockfile. Still wonder if they could be combined.
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.
Hm. Interesting.
I actually didn't think you could yarn upgrade
if you hadn't yet yarn install
ed but it seems like you can.
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.
Actually I don't think we can install all dependencies and also update a few specific dependencies in a single step. Looks like yarn upgrade <deps>
doesn't work in that case. So let's keep these separate?
(I also think it's a little easier to reason about if they are separate.)
const chalk = require('chalk'); | ||
const commandLineArgs = require('command-line-args'); | ||
const commandLineUsage = require('command-line-usage'); | ||
const figlet = require('figlet'); |
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.
such a cute name! figlet~
{ | ||
content: chalk | ||
.hex('#61dafb') | ||
.bold(figlet.textSync('react', {font: 'Graffiti'})), |
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.
This was a nice touch imo. 🎨
await Promise.all(projects.map(updateProjectPackage)); | ||
|
||
// Version sanity check | ||
await exec('yarn version-check', {cwd}); |
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.
Gotcha - at first was confused because this is similar to yarn check
but I see it's our own script here (https://github.com/facebook/react/blob/master/scripts/tasks/version-check.js) which will throw if the versions don't match. Nice.
@@ -0,0 +1,39 @@ | |||
'use strict'; |
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.
This is really beautiful. I love the structure of this whole thing.
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.
Thank you! 😁
@@ -0,0 +1,39 @@ | |||
#!/usr/bin/env node | |||
|
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.
Maybe I missed it, but could we add a prompt after the 'release' to create the 'release' on Github? This is a separate step done via the Github UI.
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.
That's part of the printed instructions once this script completes (in print-post-publish-summary
):
Step 1: Create GitHub release
- Open new release page: https://github.com/facebook/react/releases/new
- Choose ${version} from the dropdown menu
- Paste the new release notes from CHANGELOG.md
- Attach all files in build/dist/*.js except react-art.* to the release.
- Press "Publish release"!
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.
Are you planning on automating the GitHub release creation in the future too? We did this for Yarn 😄
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.
Sure. Sounds awesome. We can just copy what you've already done. 👍
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.
Let's make sure we don't release react-reconciler
with the same version as other packages. Just bumping its 0.x
minor on every release is a fine compromise for now.
…oid conflict with .gitignore
|
||
{white The CircleCI API is used to check the status of the latest commit.} | ||
{white This API requires a token which must be exposed via {yellow.bold CIRCLE_CI_API_TOKEN}} | ||
{white For instructions on creating this token check out the link below:} |
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.
Suggested wording improvements, based on our IRL convo.:
Each person using this script will need to have their own CircleCI API token.
To make this token available to the release script, you can add it to your `bash_profile` like so:
# React release script
export CIRCLE_CI_API_TOKEN=<your-token-here>
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.
@gaearon, @iamdustan: check out c6af68f Thanks for raising this concern. After this change, the release script modifies version numbers like so: diff --git a/package.json b/package.json
index 525a7d3eb..4a074f955 100644
--- a/package.json
+++ b/package.json
@@ -1,7 +1,7 @@
{
"name": "react-build",
"private": true,
- "version": "16.0.0",
+ "version": "16.1.0",
"devDependencies": {
"art": "^0.10.1",
"async": "^1.5.0",
diff --git a/packages/react-art/package.json b/packages/react-art/package.json
index 2401d764d..a5a13fd7e 100644
--- a/packages/react-art/package.json
+++ b/packages/react-art/package.json
@@ -1,7 +1,7 @@
{
"name": "react-art",
"description": "React ART is a JavaScript library for drawing vector graphics using React. It provides declarative and reactive bindings to the ART library. Using the same declarative API you can render the output to either Canvas, SVG or VML (IE8).",
- "version": "16.0.0",
+ "version": "16.1.0",
"main": "index.js",
"repository": "facebook/react",
"keywords": [
@@ -25,7 +25,7 @@
"prop-types": "^15.6.0"
},
"peerDependencies": {
- "react": "^16.0.0"
+ "react": "^16.1.0"
},
"files": [
"LICENSE",
diff --git a/packages/react-dom/package.json b/packages/react-dom/package.json
index e7e97ff57..f05ef1d9c 100644
--- a/packages/react-dom/package.json
+++ b/packages/react-dom/package.json
@@ -1,6 +1,6 @@
{
"name": "react-dom",
- "version": "16.0.0",
+ "version": "16.1.0",
"description": "React package for working with the DOM.",
"main": "index.js",
"repository": "facebook/react",
@@ -19,7 +19,7 @@
"prop-types": "^15.6.0"
},
"peerDependencies": {
- "react": "^16.0.0"
+ "react": "^16.1.0"
},
"files": [
"LICENSE",
diff --git a/packages/react-reconciler/package.json b/packages/react-reconciler/package.json
index 1432bb2ee..1f4d7b598 100644
--- a/packages/react-reconciler/package.json
+++ b/packages/react-reconciler/package.json
@@ -1,7 +1,7 @@
{
"name": "react-reconciler",
"description": "React package for creating custom renderers.",
- "version": "0.1.0",
+ "version": "0.2.0",
"keywords": [
"react"
],
@@ -20,7 +20,7 @@
"node": ">=0.10.0"
},
"peerDependencies": {
- "react": "^16.0.0"
+ "react": "^16.1.0"
},
"dependencies": {
"fbjs": "^0.8.16",
diff --git a/packages/react-test-renderer/package.json b/packages/react-test-renderer/package.json
index 0bcf56150..0faaf819b 100644
--- a/packages/react-test-renderer/package.json
+++ b/packages/react-test-renderer/package.json
@@ -1,6 +1,6 @@
{
"name": "react-test-renderer",
- "version": "16.0.0",
+ "version": "16.1.0",
"description": "React package for snapshot testing.",
"main": "index.js",
"repository": "facebook/react",
@@ -19,7 +19,7 @@
"object-assign": "^4.1.1"
},
"peerDependencies": {
- "react": "^16.0.0"
+ "react": "^16.1.0"
},
"files": [
"LICENSE",
diff --git a/packages/react/package.json b/packages/react/package.json
index 6ae473abb..e09fd6ae9 100644
--- a/packages/react/package.json
+++ b/packages/react/package.json
@@ -4,7 +4,7 @@
"keywords": [
"react"
],
- "version": "16.0.0",
+ "version": "16.1.0",
"homepage": "https://reactjs.org/",
"bugs": "https://github.com/facebook/react/issues",
"license": "MIT",
diff --git a/scripts/error-codes/codes.json b/scripts/error-codes/codes.json
index cd9a6c83d..5c494c566 100644 |
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.
My +1 assumes you will make the change Dan requested. :) |
(params.dry ? ' --dry' : ''); | ||
|
||
console.log( | ||
chalk` |
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.
Is this valid syntax? I've never seen it before.
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.
Yeah, they're called tagged template literals: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals
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.
Libs like styled-components
make heavy use of them, eg:
const Button = styled.button`
/* Adapt the colours based on primary prop */
background: ${props => props.primary ? 'palevioletred' : 'white'};
color: ${props => props.primary ? 'white' : 'palevioletred'};
font-size: 1em;
margin: 1em;
padding: 0.25em 1em;
border: 2px solid palevioletred;
border-radius: 3px;
`;
(To clarify, I haven't actually reviewed, but approved changes that previously concerned me regarding the reconciler package) |
😆 the strongest endorsement Thanks all for the feedback and suggestions! I'm going to merge this after one more quick smoke-test run locally. |
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.
🥇
**what is the change?:** We will no longer release new versions of React to Bower, and we should announce that as part of our CHANGELOG. **why make this change?:** We decided on this as a team. **test plan:** Visual inspection and spell check. :) **issue:** Just follow-up for facebook#11223
* Add note to 'unreleased' CHANGELOG about deprecating Bower **what is the change?:** We will no longer release new versions of React to Bower, and we should announce that as part of our CHANGELOG. **why make this change?:** We decided on this as a team. **test plan:** Visual inspection and spell check. :) **issue:** Just follow-up for #11223 * Improve messaging/formatting * Move bower deprecation notice to top of changelog
This script automates the release process outlined in #10620 (thanks @gaearon ❤️) but with the following deviations:
yarn build
) and generating updated error codes (yarn build -- --extract-errors
) so that we don't have to do this slow step twice.CHANGELOG
for the upcoming 16.1 release.)At a high-level, the new release script runs in 2 passes: build and publish.
The
build
script does the heavy lifting (eg checking CI, running automated tests, building Rollup bundles) and then prints instructions for manual verification. This script could be run multiple times if problems are detected during testing (although it might require some slight resetting of state unless you're using the--dry
option).The
release
script is run after manual tests. It publishes the built artifacts to NPM and pushes commits and tags to GitHub. It also prints some post-deploy instructions.Both scripts support a
--dry
flag which will log (rather than executing) commands likegit commit
ornpm publish
. This is useful for testing an upcoming build without committing to publishing it as well as for working on the release scripts themselves.These scripts also support running against a separate git checkout (to simplify the process of testing changes to the release scripts themselves).
Everybody likes pictures, so here's a few:
build
usage
successful (dry) run
example errors
publish
usage
successful (dry) run
example error
Resolves #11198