Fix error codes by keyz · Pull Request #7999 · facebook/react

keyz · 2016-10-18T02:28:57Z

I noticed that the error code in the 15-dev branch is not in sync with a freshly generated one (#7963) or in master. Specifically some invariants in src/isomorphic/hooks are missing. This is probably caused by this change while we were splitting the packages.

In this PR I copied the codes.json file from the 15-dev branch and fixed the gulp script. and regenerated error codes. For now, we only regenerate error codes when we cut a release and the master one doesn't get updated; It might be better if we sync the codes.json file back to master after each release, otherwise it'd be hard to modify an existing error message (like #7963).

Edited: please see a better process at #7999 (comment).

Reviewers:
@sebmarkbage @gaearon and cc @zpao

Test Plan:

run npm run build with the latest codes.json and there should be no warnings

sebmarkbage · 2016-10-18T03:07:47Z

gulpfile.js

+    gulp.src(paths.reactDOM.src),
+    gulp.src(paths.reactNative.src),
+    gulp.src(paths.reactTestRenderer.src)
+  ).pipe(extractErrors(errorCodeOpts));


Could you explain what's going on here? Why does this matter?

The error code generator was written in a way that it reads the file first, traverses the AST, and appends to the target file. I guess when we invoke it multiple times it causes a race condition and overwrites the same file.

To be honest there should a better way if I'm more knowledgeable in Gulp :). This PR simply changes it back to the way that the generator was used.

Yea, pretty sure that's racing. FWIW, you can probably just gulp.src([array of paths]).pipe(...) (can't test though right now so could be wrong).

keyz · 2016-10-18T16:40:23Z

@zpao thanks, I changed it to use gulp.src([].concat(...)) instead. The order of errors should be more stable this way.

keyz · 2016-10-20T23:37:47Z

Friendly ping: this needs to be fixed for the 15.4.0 release.

sebmarkbage · 2016-10-20T23:44:28Z

Which branches does this need to be merged into?

keyz · 2016-10-20T23:57:12Z

Other than the script update, I recommend merging the codes into master to unblock things like #7963. Specifically #7963 modifies an existing invariant message, and there's a test for the behavior in production so the PR author needs to regenerate the error codes.

@zpao updates the error codes when he cuts a release so only the codes in 15-dev and 15-stable get updated. I think we should sync the codes.json file back to master after each release (or just run it in master first and sync to a version branch). What do you think?

gaearon · 2016-10-21T12:08:49Z

@zpao updates the error codes when he cuts a release

I didn’t know this, and didn’t update them when cutting RCs. Do I need to?

Since @zpao is transitioning out of the team it would be great if you could provide instructions for what to do before the release, and what should not be done. Apologies if you provided them before.

gaearon · 2016-10-21T12:10:44Z

I also noticed that the error transform spews warnings every time I run npm run build on master. I wasn't sure if I'm supposed to ignore them or run extract-errors right away. By now I'm so used to this warning that I completely ignore it. What should I have done instead?

iamdustan · 2016-10-21T13:21:41Z

Is #7330 @zpao encoding his brain into a tool to manage releases in the future?

keyz · 2016-10-24T06:58:31Z

@gaearon Sorry about the delay. We've only talked about this in chats and I don't think I've formally documented it down. I just added a README for the error code system.

I also added a note for cutting a new release as follows (copied from the new README):

For each release, we run gulp react:extract-errors to update the error codes before calling npm run build. The build process uses codes.json for a production (minified) build; there should be no warning like Error message "foo" cannot be found for a successful release.

The updated codes.json file should be synced back to the master branch. The error decoder page in our documentation site uses codes.json from master; if the json file has been updated, the docs site should also be rebuilt (rake copy_error_codes is included in the default rake release task).

It's not recommended to run gulp react:extract-errors directly in master since it may contain commits that are not cherry-picked to a release branch. These error messages might be changed/removed before cutting a new release, and we don't want to add intermediate/temporary error messages to codes.json. However, if a PR changes an existing error message and there's a specific production test (which is rare), it's ok to update codes.json for that. Please use gulp react:extract-errors and don't edit the file manually.

I thought about this again and I think this is a better process for keeping everything in sync. Therefore I reverted the commit for updating codes.json in master. Are the instructions clear? Please let me know if there are still questions.

zpao · 2016-10-25T19:00:12Z

I think running in master and cherrypicking back might not be safe unless
we do it on every invariant change (could get into situation where
invariant added in A and changed in B but only A is cherry-picked for
release in 15). So I've just been doing it on the releases (but should also
do it for RCs, sorry for missing that). We should cherry pick that back to
master though.

On Friday, October 21, 2016, Dustan Kasten notifications@github.com wrote:

Is #7330 #7330 @zpao
https://github.com/zpao encoding his brain into a tool to manage
releases in the future?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7999 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAAg_dmm10Q6XjE4zGfYRdkRKSDnzk90ks5q2LxpgaJpZM4KZT3w
.

keyz · 2016-10-25T21:02:12Z

@zpao Thanks! Yeah I thought about it again and I agree with you (there's an updated workflow at #7999 (comment)).

keyz · 2016-10-25T21:12:46Z

@gaearon Just saw your updates in #8033. Just making sure, did you intend to let the script collect error messages in reactNoopRenderer (https://github.com/facebook/react/pull/8033/files#diff-b9e12334e9eafd8341a6107dd98510c9R274)? Since reactNoopRenderer is not publicly released yet and no one uses its prod build, I recommend not to add it for now to avoid adding intermediate/temporary error messages to codes.json.

This reverts commit b8f3aee.

keyz · 2016-10-25T21:23:13Z

gulpfile.js

-    gulp.src(paths.reactDOM.src).pipe(extractErrors(errorCodeOpts)),
-    gulp.src(paths.reactNative.src).pipe(extractErrors(errorCodeOpts)),
-    gulp.src(paths.reactTestRenderer.src).pipe(extractErrors(errorCodeOpts)),
-    gulp.src(paths.reactNoopRenderer.src).pipe(extractErrors(errorCodeOpts))


@gaearon I rebased and removed reactNoopRenderer from the list of extract-errors targets. Please let me know if it makes sense!

Yea, totally. I was just copy pasting.

gaearon · 2016-10-27T09:40:30Z

Keyan, what is my next step? Merge this in master and cherry pick to 15-stable and 15-dev?

keyz · 2016-10-27T14:48:51Z

@gaearon Yeah, for this PR, let's:

Merge this PR in master
Cherry pick to 15-stable and 15-dev

When you cut a release, let's do:

Run gulp react:extract-errors in 15-dev
If codes.json gets updated, please cherry pick it back to master and 15-stable
npm run build and other release tasks

* took codes.json from the 15-dev branch * fixed react:extract-errors task in gulpfile * generated error codes * Revert "generated error codes" This reverts commit b8f3aee. * Added a README for the error code system

facebook-github-bot added GH Review: review-needed CLA Signed labels Oct 18, 2016

sebmarkbage reviewed Oct 18, 2016

View reviewed changes

keyz force-pushed the fix-error-codes branch from bc75a52 to b8f3aee Compare October 18, 2016 16:37

facebook-github-bot added the CLA Signed label Oct 18, 2016

sebmarkbage added GH Review: accepted and removed GH Review: review-needed labels Oct 20, 2016

facebook-github-bot added the CLA Signed label Oct 21, 2016

facebook-github-bot added the CLA Signed label Oct 22, 2016

keyz force-pushed the fix-error-codes branch 2 times, most recently from 6237ec2 to d7347d2 Compare October 24, 2016 06:51

keyz added GH Review: review-needed and removed GH Review: accepted labels Oct 24, 2016

facebook-github-bot added the CLA Signed label Oct 24, 2016

chicoxyzzy mentioned this pull request Oct 24, 2016

Add npm v4.0.0 support #8082

Merged

keyz force-pushed the fix-error-codes branch from d7347d2 to b3b103e Compare October 25, 2016 21:03

keyz added 3 commits October 25, 2016 17:19

took codes.json from the 15-dev branch

497f29d

fixed react:extract-errors task in gulpfile

c2af551

generated error codes

7f86329

keyz added 2 commits October 25, 2016 17:20

Revert "generated error codes"

554db9a

This reverts commit b8f3aee.

Added a README for the error code system

775db3c

keyz force-pushed the fix-error-codes branch from b3b103e to 775db3c Compare October 25, 2016 21:21

keyz commented Oct 25, 2016

View reviewed changes

keyz mentioned this pull request Oct 28, 2016

how remove debug info from react.min.js file when publish project #7990

Closed

gaearon merged commit 46609cb into facebook:master Nov 8, 2016

gaearon mentioned this pull request Nov 8, 2016

Ship 15.4.0 #7770

Closed

gaearon mentioned this pull request Jan 11, 2017

Add error codes update to release process #8749

Merged

Conversation

keyz commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sebmarkbage Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

keyz Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

zpao Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

keyz commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyz commented Oct 20, 2016

Uh oh!

sebmarkbage commented Oct 20, 2016

Uh oh!

keyz commented Oct 20, 2016

Uh oh!

gaearon commented Oct 21, 2016

Uh oh!

gaearon commented Oct 21, 2016

Uh oh!

iamdustan commented Oct 21, 2016

Uh oh!

keyz commented Oct 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zpao commented Oct 25, 2016

Uh oh!

keyz commented Oct 25, 2016

Uh oh!

keyz commented Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyz Oct 25, 2016

Choose a reason for hiding this comment

Uh oh!

gaearon Oct 27, 2016

Choose a reason for hiding this comment

Uh oh!

gaearon commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

keyz commented Oct 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

keyz commented Oct 18, 2016 •

edited

Loading

keyz commented Oct 18, 2016 •

edited

Loading

keyz commented Oct 24, 2016 •

edited

Loading

keyz commented Oct 25, 2016 •

edited

Loading

gaearon commented Oct 27, 2016 •

edited

Loading