Display which invalid type was returned #7963

davidblurton · 2016-10-12T22:43:50Z

facebook-github-bot · 2016-10-12T22:44:49Z

Thank you for your pull request. As you may know, we require contributors to sign our Contributor License Agreement, and we don't seem to have you on file and listed as active anymore. In order for us to review and merge your code, please email cla@fb.com with your details so we can update your status.

facebook-github-bot · 2016-10-12T23:11:42Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

aweary · 2016-10-13T02:26:12Z

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

+    return 'null';
+  }
+  if (Array.isArray(element)) {
+    return 'array';


Can you return 'an array' so that the error message sounds less awkward?

gaearon · 2016-10-13T09:52:43Z

src/renderers/shared/stack/reconciler/ReactCompositeComponent.js

@@ -47,13 +47,24 @@ StatelessComponent.prototype.render = function() {
  return element;
 };

+function getTypeof(element) {


Let's call this getElementTypeForWarning.

davidblurton · 2016-10-13T09:57:02Z

Updated with requested changes.

davidblurton · 2016-10-15T17:10:06Z

@gaearon updated with your suggestions and tests fixed :)

gaearon · 2016-10-15T17:16:33Z

scripts/error-codes/codes.json

@@ -104,11 +104,11 @@
  "102": "EventPluginRegistry: Cannot inject two different event plugins using the same name, `%s`.",
  "103": "executeDirectDispatch(...): Invalid `event`.",
  "104": "ReactCompositeComponent: injectEnvironment() can only be called once.",
-  "105": "%s(...): A valid React element (or null) must be returned. You may have returned undefined, an array or some other invalid object.",
+  "105": "%s(...): A valid React element (or null) must be returned, but you returned %s.",


English nit: comma before but is not necessary? (I'm not actually a native speaker so correct me if I'm wrong.)

If the two clauses joined by but are independent (test: can they stand alone and make sense as a sentence?) a comma is appropriate. So a comma is correct here

gaearon · 2016-10-15T17:18:10Z

Can you please run the docs locally (instructions are in docs folder), and verify that the error decoder correctly decodes the link locally? A screenshot showing this would be nice.

keyz · 2016-10-15T17:41:42Z

Hi @davidblurton,

Thanks for your contribution. However, the codes.json file is automatically generated and should never be edited manually. Instead, please run ./node_modules/.bin/gulp react:extract-errors to update the json files -- the new/updated errors will be appended to the end of codes.json, which ensures our decoder page stays backwards compatible.

davidblurton · 2016-10-15T23:06:19Z

I changed the error message to something better:

Component.render() must return a valid React element (or null). You returned an array.

davidblurton · 2016-10-15T23:07:16Z

Here's a screenshot of the generated docs:

davidblurton · 2016-10-17T21:28:12Z

@gaearon is this good to go?

keyz · 2016-10-17T22:13:10Z

Sorry, I just noticed that the error code in the 15-dev branch (https://github.com/facebook/react/blob/15-dev/scripts/error-codes/codes.json) is not in sync with the one here or in master. Specifically some invariants in src/isomorphic/hooks are missing/duplicated. It's probably because we only generate the error code when we cut a release, so the one in master doesn't get updated. @davidblurton can you give us some time to resolve this issue first? I'll create a separate issue.

aweary

@davidblurton can you resolve the merge conflicts please? I'll make sure this gets merged if we can do it cleanly 👍

aweary · 2017-03-02T19:45:32Z

@davidblurton sorry about this, looks like some more conflicts. Can you address those?

davidblurton · 2017-03-02T19:52:03Z

@aweary I'm getting strange errors like this and I don't know how to fix them. Do you know what the cause of this is?

 FAIL  src/renderers/dom/__tests__/ReactDOMProduction-test.js
  ● ReactDOMProduction › should throw with an error code in production

    TypeError: process.cwd is not a function
      
      at printActualErrorMessage (node_modules/jest-matchers/build/toThrowMatchers.js:153:26)
      at Object.<anonymous> (src/renderers/dom/__tests__/ReactDOMProduction-test.js:191:8)

aweary · 2017-03-02T19:56:55Z

@davidblurton can you rebase with master, reinstall node_modules and try again?

gaearon · 2017-04-14T19:01:44Z

scripts/error-codes/codes.json

We shouldn't update error codes in a PR.

I almost removed it when I rebased for @davidblurton, but it wasn't clear if this should be excluded. There is a test that relies on testing the new error (and error code) in ReactDOMProduction, won't that test potentially fail if codes.json isn't updated now since the code will potentially change?

If the test doesn't pass, can you amend the test instead? If it specifically tests for "error codes" then we can change it to trigger some other invariant whose code didn't change.

Oh I see now, it's not testing this specific invariant it's just testing any production invariant. That makes sense, we should definitely change it.

gaearon · 2017-04-14T19:02:45Z

src/renderers/shared/fiber/ReactChildFiber.js

@@ -73,7 +73,20 @@ const {
  Deletion,
 } = ReactTypeOfSideEffect;

-function coerceRef(current: Fiber | null, element: ReactElement) {
+function getElementTypeForWarning(element) {


This is not a warning though, it's an error.

gaearon · 2017-04-14T19:42:30Z

src/renderers/shared/fiber/ReactChildFiber.js

+    return 'null';
+  }
+  if (element === undefined) {
+    return 'undefined';


Can we avoid strings in these two cases? Seems like return '' + element would be enough. Minor thing but minifies better.

gaearon · 2017-10-04T23:16:59Z

Really sorry about how we handled this. We were in the middle of a rewrite of the whole reconciler, and it wasn't clear which features will be supported by the time we finish. In the end we support both strings, numbers, and arrays, that we don't really need this change. I really appreciate the effort you put into it, and I hope this doesn't discourage you from contributing in the future.

facebook-github-bot added the GH Review: review-needed label Oct 12, 2016

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

aweary suggested changes Oct 13, 2016

View reviewed changes

gaearon reviewed Oct 13, 2016

View reviewed changes

davidblurton force-pushed the let-users-know-which-invalid-type-was-returned branch from 1d3b1b7 to 999dc36 Compare October 13, 2016 09:54

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

aweary approved these changes Oct 13, 2016

View reviewed changes

gaearon reviewed Oct 15, 2016

View reviewed changes

gaearon self-assigned this Oct 15, 2016

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

gaearon added GH Review: needs-revision and removed GH Review: review-needed labels Oct 15, 2016

facebook-github-bot added CLA Signed GH Review: review-needed and removed GH Review: needs-revision labels Oct 15, 2016

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

keyz mentioned this pull request Oct 18, 2016

Fix error codes #7999

Merged

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

aweary suggested changes Jan 26, 2017

View reviewed changes

aweary assigned aweary and unassigned gaearon Jan 26, 2017

davidblurton force-pushed the let-users-know-which-invalid-type-was-returned branch from d3679d8 to e50e618 Compare January 28, 2017 23:21

sebmarkbage removed the GH Review: review-needed label Mar 1, 2017

davidblurton force-pushed the let-users-know-which-invalid-type-was-returned branch from 8daa72d to b71c6f9 Compare March 2, 2017 21:45

davidblurton and others added 9 commits April 14, 2017 11:52

Display which invalid type was returned

2174cae

Improve readability of error message

b3b5cd7

Minified error message args must be url encoded

6f60927

Reset changes to error-code

ace0e02

Rewrite error message avoiding passive voice and dubious commas.

7f64429

Update test with new error code

85bffb8

Update tests with new error messages

f729ff2

Use new error message in fiber

619b467

Update error codes and production test assertion

a676b18

aweary force-pushed the let-users-know-which-invalid-type-was-returned branch from b71c6f9 to a676b18 Compare April 14, 2017 17:20

Run prettier

3cc0eac

gaearon reviewed Apr 14, 2017

View reviewed changes

gaearon closed this Oct 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display which invalid type was returned #7963

Display which invalid type was returned #7963

davidblurton commented Oct 12, 2016

facebook-github-bot commented Oct 12, 2016

facebook-github-bot commented Oct 12, 2016

aweary Oct 13, 2016

gaearon Oct 13, 2016

davidblurton commented Oct 13, 2016

davidblurton commented Oct 15, 2016

gaearon Oct 15, 2016

sethkinast Oct 15, 2016

gaearon commented Oct 15, 2016

keyz commented Oct 15, 2016

davidblurton commented Oct 15, 2016

davidblurton commented Oct 15, 2016

davidblurton commented Oct 17, 2016

keyz commented Oct 17, 2016 •

edited

Loading

aweary left a comment

aweary commented Mar 2, 2017

davidblurton commented Mar 2, 2017

aweary commented Mar 2, 2017

gaearon Apr 14, 2017

aweary Apr 14, 2017

gaearon Apr 14, 2017

aweary Apr 14, 2017

gaearon Apr 14, 2017

gaearon Apr 14, 2017

gaearon commented Oct 4, 2017

Display which invalid type was returned #7963

Display which invalid type was returned #7963

Conversation

davidblurton commented Oct 12, 2016

facebook-github-bot commented Oct 12, 2016

facebook-github-bot commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidblurton commented Oct 13, 2016

davidblurton commented Oct 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Oct 15, 2016

keyz commented Oct 15, 2016

davidblurton commented Oct 15, 2016

davidblurton commented Oct 15, 2016

davidblurton commented Oct 17, 2016

keyz commented Oct 17, 2016 • edited Loading

aweary left a comment

Choose a reason for hiding this comment

aweary commented Mar 2, 2017

davidblurton commented Mar 2, 2017

aweary commented Mar 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaearon commented Oct 4, 2017

keyz commented Oct 17, 2016 •

edited

Loading