feat: allow direct access to caught error and errorInfo

[davwheat]

The error value returned by the hook call is a boolean, not the actual caught error.

The TS typings reflect this, but the readme documentation does not.

[tatethurston]

Thanks for opening this @davwheat. This is indeed an oversight -- I think it would be more useful to update the source code to set the actual caught error, rather than a boolean. I would be happy to accept a PR for that change. Otherwise, I'll patch and publish this evening.

[davwheat]

That sounds more reasonable, but I would worry about the behaviour and effects this could have if the code, for whatever reason, threw null or undefined or some other falsey value, which would break if (error) checks.

Ideally, it could return an object, or the hook could return a third array element in order to not be a breaking change?

[tatethurston]

It is a breaking change, but we can reflect that in the package versioning and bump to 2.0.0. If any users file a request for throwing null or undefined we can evaluate support then.

[davwheat]

The reason I mentioned it was that some of my (old and terrible) code does throw falsey values, using TS enums as error codes. In that scenario, anything throwing 0 (one of my enum values) would act as if there's no error at all.

Looking at the code in more detail, I propose that error could be either undefined or an object containing both the error and React's errorInfo. This way, we can also access the error's component stack from the hook, too, and reduce the breaking-ness of the change (truthy/falsey checks will still work -- only direct comparisons against true or false will break).

What do you think of this?

Example:

function MyComponent() {
  const [errorData, resetError] = useErrorBoundary();

  // undefined, or object containing error data
  if (errorData) {
    const { error, errorInfo } = errorData;

    return <p>{error.message}</p>;
  }

  return null;
}

[codecov-commenter]

Codecov Report

Merging #9 (04b3281) into main (cc4b150) will decrease coverage by 20.00%.
The diff coverage is 46.15%.

@@             Coverage Diff              @@
##              main       #9       +/-   ##
============================================
- Coverage   100.00%   80.00%   -20.00%     
============================================
  Files            1        1               
  Lines           27       35        +8     
  Branches         0        1        +1     
============================================
+ Hits            27       28        +1     
- Misses           0        7        +7     

Impacted Files	Coverage Δ
src/index.tsx	80.00% <46.15%> (-20.00%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f654ccf...04b3281. Read the comment docs.

[tatethurston]

errorInfo is a good call out, and something I had not considered. This is necessary for a lot of react reporting tool integrations (like sentry).

I think we should update the callback argument to include errorInfo:

  const [error, resetError] = useErrorBoundary(
    // You can also log the error to an error reporting service
    (error, errorInfo) => logErrorToMyService(error, errorInfo)
  );

I'm curious if errorInfo is still desirable in the return value after that change. The symmetry is nice.

I like the ergonomics of having direct access to the error that was thrown, which I suspect is the dominant use case.

How burdensome would updating your existing callsites to if (error != undefined) be?

[davwheat]

errorInfo seems to already be included as an argument, but is just undocumented within the README, as all arguments are passed to that callback:

react-use-error-boundary/src/index.tsx

Lines 67 to 70 in 74d81ec

	onError={(...args) => {
	setError(true);
	componentDidCatch.current?.(...args);
	}}

I'll push the new code I have based on my previous comment, and let you see what you think. I do think that the same values should be available through the hook as are available through the onError callback.

How burdensome would updating your existing callsites to if (error != undefined) be?

It wouldn't be horrible, but I do still think that a simple falsey check looks cleaner, in my opinion.

[davwheat]

I chose unknown here as we can't guarantee the thrown value will be an instance of Error, and I think enforcing type safety is more important than convenience, if this is meant to be a back-up/fail-safe for other errors.

Using instanceof Error would be enough to please Typescript into allowing access to Error's attributes.

[tatethurston]

I'd like to hold off on adding errorInfo to the return value until a concrete use case materializes. I suspect the error callback will be sufficient (because the component stack will only be used for error reporting), and the return value will only used for rendering error specific UI.

With that, I'm still inclined to update the source code to set the caught error, rather than a boolean.

I could see using instanceof Error internally, and wrapping any thrown primitives in an Error to solve for the throw falsey values:

eg

if (!error instanceof Error) {
  error = new Error(error);
}

[tatethurston]

Suggested change

	type UseErrorBoundaryReturn = [hasErrored: ErrorType, resetError: () => void];
	type UseErrorBoundaryReturn = [error: Error, resetError: () => void];

[tatethurston]

Suggested change

	onError={(error: Error | WrappedError, errorInfo) => {
	onError={(error: unknown, errorInfo) => {

Could you add the type annotation for errorInfo?

[davwheat]

This is already covered in ErrorBoundaryProps at the top.

[tatethurston]

The type here should be unknown right?

[tatethurston]

Suggested change

	const [error, setError] = useState<ErrorType>(undefined);
	const [error, setError] = useState<Error>(undefined);

[tatethurston]

Suggested change

	error: ErrorType;
	setError: (error: ErrorType) => void;
	error: Error;
	setError: (error: Error) => void;

[tatethurston]

Suggested change

	error: ErrorType;
	error: Error;

[tatethurston]

Suggested change

type ErrorType = WrappedError | Error | undefined;

[davwheat]

My reasoning behind this was to more clearly annotate the types for users, so that they know they can import WrappedError and use instanceof to check if .originalData will be present.

I think we should still keep the | undefined because when using TS in strict mode, undefined and null are no longer ignored by TS, and this could cause unexpected errors for users of the package with TS in strict mode (error === undefined will be interpreted as never as TS doesn't know this might be undefined).

[tatethurston]

Yes on the call sites where undefined is necessary -- I was thinking we could inline these.

My inclination is to keep WrappedError private to the package for now. The property accessor is a nice convenience for local debugging.

[tatethurston]

Suggested change

	let stringifiedData =
	"It was not possible to parse the data thrown as a string.";
	/*
	Some values cannot be converted into a string, such as Symbols
	or certain Object instances (e.g., `Object.create(null)`).
	This try/catch ensures that our silent error wrapper doesn't
	cause an unexpected error for the user, bricking the React app
	when we're meant to be preventing errors doing so.
	*/
	try {
	stringifiedData = String(data);
	} catch {
	// Ignore errors
	}
	super(stringifiedData);
	/*
	Some values cannot be converted into a string, such as Symbols
	or certain Object instances (e.g., `Object.create(null)`).
	This try/catch ensures that our silent error wrapper doesn't
	cause an unexpected error for the user, bricking the React app
	when we're meant to be preventing errors doing so.
	*/
	try {
	super(data);
	} catch {
	super("react-use-error-boundary: The thrown value could not be used to instantiate an instance of Error. The original thrown value can be accessed via the originalData property.")
	}
	this.name = 'WrappedError'