React: Finesse signature of componentDidCatch in React 16

[chris-kobrzak]

Do you want to request a feature or report a bug?

Improvement to a new feature

What is the current behaviour?

I believe the proposed name componentDidCatch is a little confusing.

What is the expected behavior?

In a nutshell, this proposal is about changing the signature from

componentDidCatch(error, info)

to

componentDidCatchError(error, message)

Naming conventions are often half-jokingly labelled as one of the most difficult things in computer
science but I think we could easily tweak the signature of the componentDidCatch lifecycle method
that is being introduced in React 16.

I am heavily inspired by the principles covered in the famous Clean Code book by Robert C. Martin
and I particularly like the idea of solid naming conventions where you can basically _guess_ what a
function does or what sort of variable you are dealing with by literally reading their names. Yes, every
software developer in the world will associate the term "catch" with the try/catch construct but
the first question that came to my mind when I came across componentDidCatch was something
along the lines of "catch what?".

So I think we could avoid this confusion and make the new method a bit more intuitive if we:

1. renamed it to either componentDidCatchError or componentDidCatchException and
2. renamed the second argument to something that would better indicate we are expecting a string,
   e.g. message or errorMessage. I just find the term "info" that you have used in your recent
   blog post a little vague as it's not immediately obvious whether it is meant to be a string or maybe
   an object.

And before I forget, the actual functionality of this new method seems great and I can't wait for
an opportunity to implement it in my projects!

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 16

[gaearon]

The second argument is kind of intentionally vague because we haven’t quite decided what goes in there yet. For now it’s just {componentStack: string} but we might add more things in the future so we went with passing an object.

As for the first point.. It’s subjective. Some people will see componentDidCatch as too short, others will see componentDidCatchError as too long. We have to pick something, and we went with this one. I think the parallel between catch(error) {} and componentDidCatch(error) is reasonable enough.

[chris-kobrzak]

OK, fair enough - info actually _feels_ more like an object so if you are intending to use it for that
data type, that makes perfect sense.

So the amended proposal would therefore be:

componentDidCatchError(error, info)

Thanks!

[aweary]

componentDidCatch will be triggered if you throw anything, not just an error (example). With that in mind, componentDidCatch is a more accurate name since there's no guarantee that it's catching an error, it could be anything that could be thrown.

[chris-kobrzak]

@aweary, are we not overly complicating things a little bit here? If I understand the new lifecycle method correctly, it is meant to catch any _exceptions_ thrown in any of the descendant components.

Just like in your example where you're using the throw statement in a child component and the result is captured in the parent. Now, according to the throw statement documentation, it

throws a user-defined exception

So it doesn't matter whether you throw an Error object or an integer (as per your example), what matters is it _is_ an exception. It is perhaps debatable whether it's a good thing that this exception can be of any type but this is the way JavaScript is.

I guess all that convinces me the optimal solution would therefore be:

componentDidCatchException(Any: exception, Object: info)

[sebmarkbage]

I love incorporating the expected argument name in how you read it. If you only read the words it reads as: "component did catch error" because you have an error argument. Otherwise it would read as "component did catch error error".

Similarly, we have a not-yet-documented API called "flushSync(batch)" which reads as "flush sync batch" if you name the argument. Otherwise it would be "flushSyncBatch(batch)" which reads as "flush sync batch batch". :)