Allow Promise to display error strings and not just error objects. #9989

mikelambert · 2016-09-19T23:26:40Z

No description provided.

ghost · 2016-09-19T23:26:54Z

By analyzing the blame information on this pull request, we identified @davidaurelio and @danieldunderfelt to be potential reviewers.

eslint-bot · 2016-09-19T23:32:29Z

Libraries/Promise.js

@@ -18,6 +18,9 @@ if (__DEV__) {
    allRejections: true,
    onUnhandled: (id, error = {}) => {
      const {message = null, stack = null} = error;
+      if (typeof error === 'string' || error instanceof String) {
+        message = error;


message const cannot be reassigned const message

ide · 2016-09-20T00:36:45Z

Better to handle Errors specially and then have a fallback for all non-Error types ex: with https://github.com/thejameskyle/pretty-format

ghost · 2016-09-20T01:27:33Z

@mikelambert updated the pull request - view changes

mikelambert · 2016-09-20T01:29:45Z

You want me to add another dependency to react-native? I can do that if desired, just figured a simple change that handled the (what I assumed, only) case of strings was a better and easier to get merged.

ide · 2016-09-20T06:28:09Z

It's easier to get merged but more arbitrary and handling all values seems better to me.

davidaurelio · 2016-09-20T10:14:21Z

Libraries/Promise.js

+      let {message = null, stack = null} = error;
+      if (typeof error === 'string' || error instanceof String) {
+        message = error;
+      }


What about `if (message == null) { message = String(error); }

davidaurelio · 2016-09-20T10:15:29Z

Libraries/Promise.js

+      let {message = null, stack = null} = error;
+      if (typeof error === 'string' || error instanceof String) {
+        message = error;
+      }
      const warning =
        `Possible Unhandled Promise Rejection (id: ${id}):\n` +
        (message == null ? '' : `${message}\n`) +


or here: ${message == null ? String(error) : message}\n

davidaurelio · 2016-09-20T10:17:12Z

pretty-format would work, too, of course

mikelambert · 2016-09-20T10:34:24Z

Thanks David, I'm partial to String(error) until we find errors raised that warrant more, but let me know if you prefer pretty-format, and I'll make the change.

And sorry if I'm mistaken @ide , but I'm assuming you're an external dev chiming in but not a core RN committer, so am deferring to @davidaurelio on the pretty-format dependency. Otherwise I'm not sure how to weight your preference on this change. :)

davidaurelio · 2016-09-20T10:48:43Z

@mikelambert sounds good. @ide is as core as it gets 😉

ghost · 2016-09-23T23:30:02Z

@mikelambert updated the pull request - view changes

ghost · 2016-09-23T23:39:16Z

@mikelambert updated the pull request - view changes

mikelambert · 2016-09-23T23:40:41Z

Sorry for the fb-bot spam, I'm dealing with this git learning curve on my end, and screwed up my branches. This should be back to the original pull again (sorry, haven't had made time yet to make the pretty-format change and test it yet)

…o errors

ghost · 2016-09-23T23:42:53Z

@mikelambert updated the pull request - view changes

facebook-github-bot · 2016-10-24T17:01:27Z

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @danieldunderfelt as a potential reviewer. Could you take a look please or cc someone with more context?

mikelambert · 2016-11-08T05:27:56Z

Sorry @hramos. I've just updated with the latest set of changes @ide suggested. I've tested that it works and logs correctly with:
throw 'str';
throw new Error('str');
throw new RangeError('str');

ide · 2016-11-08T08:44:40Z

@mikelambert thanks, this is robust and simple! The CircleCI tests look like they are failing because of a JDK issue, unrelated to this PR. I don't think there are automated tests for promise rejections so we aren't missing much signal from CircleCI plus you said you manually tested it.

@hramos Could you help import this PR? It adds a dependency (https://github.com/thejameskyle/pretty-format).

facebook-github-bot · 2016-11-08T15:39:53Z

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mikelambert · 2016-11-26T09:17:03Z

What is the next step for this change? @hramos imported it 18 days ago, but it is not yet in master. Is it sitting internally waiting in phabricator for review?

mikelambert · 2016-12-12T05:59:35Z

Ping?

mikelambert · 2017-01-23T08:58:09Z

Ping @ide @hramos ?

ide · 2017-01-23T22:38:55Z

@ericvicenti can you import and commit this?

facebook-github-bot · 2017-01-23T22:59:03Z

@ericvicenti has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-01-24T12:48:15Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

mikelambert · 2017-01-25T10:04:10Z

I've updated and fixed a merge conflict on package.json, so hopefully this should be safe to try again, Mr. @facebook-github-bot .

Sorry about the commit-log mess. My git-foo is not good enough to rebase it properly without messing up the entire branch. Will your phabricator import collapse it all? If not...I can make an attempt at rebasing and collapsing it, and worst-case submit a new clean pull request.

Thanks!

mikelambert · 2017-01-25T23:37:23Z

Ping @ericvicenti since I re-merged master, and suspect it needs re-triggering on your end.

facebook-github-bot · 2017-01-26T01:14:51Z

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-01-26T15:48:12Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

mikelambert · 2017-01-27T00:30:03Z

@hramos and @ericvicenti to clarify if this is something I need to act on or not...

It passes all checks according to github
I just recently merged to github master, dunno if I already need to do it again?
I'm assuming it instead must be breaking some internal test?

hramos · 2017-01-27T00:57:06Z

@mikelambert nothing needed on your part. Anything that touches package.json requires a manual import on our side. We have already done that part, but the internal diff is failing some internal tests. It looks like the failures are transient and unrelated. I'll kick it off again and see if it passes tests again.

Update: Actually one of the test failures is related to this diff, it looks like we need to update some things on our side to account for the additional packages.

hramos · 2017-03-10T03:13:15Z

Finally!

ghost added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Sep 19, 2016

eslint-bot reviewed Sep 19, 2016

View reviewed changes

Allow Promise to display error strings and not just error objects.

215a41b

mikelambert force-pushed the master branch from 05ade5a to 215a41b Compare September 20, 2016 01:26

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 20, 2016

davidaurelio reviewed Sep 20, 2016

View reviewed changes

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 20, 2016

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 23, 2016

mikelambert force-pushed the master branch from ff7c85e to 215a41b Compare September 23, 2016 23:39

ghost added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 23, 2016

Merge branch 'master' of https://github.com/facebook/react-native int…

6f6403d

…o errors

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 23, 2016

mikelambert added 2 commits October 29, 2016 14:51

Use pretty-format for error printing.

3a476f5

Use pretty-format for error printing.

5601eee

mikelambert force-pushed the master branch from c2f8691 to 88ea195 Compare November 8, 2016 05:25

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 8, 2016

ide added GH Review: accepted and removed GH Review: review-needed labels Nov 8, 2016

ide approved these changes Nov 8, 2016

View reviewed changes

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 23, 2017

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jan 24, 2017

mikelambert added 2 commits January 25, 2017 18:56

Merge branch 'master' of https://github.com/mikelambert/react-native

352abf5

Merge branch 'master' of https://github.com/facebook/react-native

323bd5b

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 26, 2017

facebook-github-bot removed the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 26, 2017

facebook-github-bot closed this in bfb2766 Mar 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Promise to display error strings and not just error objects. #9989

Allow Promise to display error strings and not just error objects. #9989

mikelambert commented Sep 19, 2016

ghost commented Sep 19, 2016

eslint-bot Sep 19, 2016

ide commented Sep 20, 2016

ghost commented Sep 20, 2016

mikelambert commented Sep 20, 2016

ide commented Sep 20, 2016

davidaurelio Sep 20, 2016

davidaurelio Sep 20, 2016

davidaurelio commented Sep 20, 2016

mikelambert commented Sep 20, 2016

davidaurelio commented Sep 20, 2016

ghost commented Sep 23, 2016

ghost commented Sep 23, 2016

mikelambert commented Sep 23, 2016

ghost commented Sep 23, 2016

facebook-github-bot commented Oct 24, 2016

mikelambert commented Nov 8, 2016

ide commented Nov 8, 2016

facebook-github-bot commented Nov 8, 2016

mikelambert commented Nov 26, 2016

mikelambert commented Dec 12, 2016

mikelambert commented Jan 23, 2017

ide commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

facebook-github-bot commented Jan 24, 2017

mikelambert commented Jan 25, 2017

mikelambert commented Jan 25, 2017

facebook-github-bot commented Jan 26, 2017

facebook-github-bot commented Jan 26, 2017

mikelambert commented Jan 27, 2017

hramos commented Jan 27, 2017 •

edited

Loading

hramos commented Mar 10, 2017

Allow Promise to display error strings and not just error objects. #9989

Allow Promise to display error strings and not just error objects. #9989

Conversation

mikelambert commented Sep 19, 2016

ghost commented Sep 19, 2016

eslint-bot Sep 19, 2016

Choose a reason for hiding this comment

ide commented Sep 20, 2016

ghost commented Sep 20, 2016

mikelambert commented Sep 20, 2016

ide commented Sep 20, 2016

davidaurelio Sep 20, 2016

Choose a reason for hiding this comment

davidaurelio Sep 20, 2016

Choose a reason for hiding this comment

davidaurelio commented Sep 20, 2016

mikelambert commented Sep 20, 2016

davidaurelio commented Sep 20, 2016

ghost commented Sep 23, 2016

ghost commented Sep 23, 2016

mikelambert commented Sep 23, 2016

ghost commented Sep 23, 2016

facebook-github-bot commented Oct 24, 2016

mikelambert commented Nov 8, 2016

ide commented Nov 8, 2016

facebook-github-bot commented Nov 8, 2016

mikelambert commented Nov 26, 2016

mikelambert commented Dec 12, 2016

mikelambert commented Jan 23, 2017

ide commented Jan 23, 2017

facebook-github-bot commented Jan 23, 2017

facebook-github-bot commented Jan 24, 2017

mikelambert commented Jan 25, 2017

mikelambert commented Jan 25, 2017

facebook-github-bot commented Jan 26, 2017

facebook-github-bot commented Jan 26, 2017

mikelambert commented Jan 27, 2017

hramos commented Jan 27, 2017 • edited Loading

hramos commented Mar 10, 2017

hramos commented Jan 27, 2017 •

edited

Loading