Pass error stack from call site to mutable warnings

[louis-openspace]

Summary

Introduced in Reanimated 3.16, the "Writing/Reading to value during component render" warning is very difficult to debug because the error stack is created through several layers of log calls. By passing the error stack from the call site where the mutable is called, users can pinpoint the site where the mutable was invalidly read/write at the second stack trace line.

Test plan

1. Write or read to a mutable value in a render function, the second stack trace line should be the call site where the error occured.

[piaskowyk]

Interesting changes - thanks for your PR and time! 🙌

[piaskowyk]

What do you think about moving Error().stack inside the if condition to avoid unnecessary creation of Error() instances and potential performance regressions?

- function checkInvalidWriteDuringRender(stack?: string) {
+ function checkInvalidWriteDuringRender() {
  if (shouldWarnAboutAccessDuringRender()) {
    logger.warn(
      '!!Writing to `value` during component render. Please ensure that you do not access the `value` property or use `set` method of a shared value while React is rendering a component.',
      { strict: true },
-     { stack }
+     { stack: Error().stack }

It would be ok for you?

[MatiPl01]

This change will add one more frame in the call stack, so I think that the Error object should be crated as close to the function exposed from reanimated (in this case - value getter/setter) as possible.

I don't think that moving error object anywhere else is a valid solution, though. It will introduce more hassle and doesn't address the real problem. See my comment below explaining why.

[louis-openspace]

@piaskowyk from skimming hermes code I think the .stack is a getter that will format a string so that would be slow. I think creating the Error() itself might be cheap and then I could only pass the error reference itself and access .stack later.

[MatiPl01]

Hey @louis-openspace!

Thank you for your PR.

The invalid read/write warning introduced in reanimated 3.16 was intended to work together with the new wrapWithReanimatedMetroConfig function which should filter out unwanted frames from the warning call stack. Unfortunately, after logs in React Native were moved from metro to the new web-based debugger, these unwanted stack frames aren't filtered out, thus you end up seeing calls from reanimated internals in your stack trace.

Even though your solution removes some unnecessary stack frames, it is not ideal, as it won't add much benefit to warnings and errors logged from other functions in reanimated, which are deeper in the call stack. It will always remove just these few logs related to the reanimated logger and keep all calls up to the point where the new Error() was called.

I think we should investigate the problem and find a valid solution for the new react native debugger instead of fixing the problem just in part in a way proposed in this PR.

[louis-openspace]

@MatiPl01 thanks, I'd be interested if you find a workaround with the RN debugger. This method of forwarding error stacks I got from @testing-library where we aren't interested in stack traces and async stack frames from the testing library itself but from our own code that caused the error. I'm confused why it is not ideal, as I only saw the stacks I forwarded used in these two read/write warnings

[MatiPl01]

I'd be interested if you find a workaround with the RN debugger.

I will look into recent changes in the react native logging system and will try to adapt logger in reanimated to be capable of displaying clean stack traces. I will let you know if I find something that works.

I'm confused why it is not ideal, as I only saw the stacks I forwarded used in these two read/write warnings

By saying that this approach is not ideal, I meant that you always have to create the Error object as close to the function called by the user as possible.

So, let's say we have exported function a from the library that calls b, which then calls c and we are logging a warning in c. To remove unwanted stack frames, we would have to create the error object in the a function and pass it through b to c in order to pass it in options to the logger function. I don't like this pattern and it would require many changes in our existing code if we would decide to implement it for all remaining warnings in the library.

It works fine in your case, as the value getter and setter is called directly by the user and you have to pass the error stack only one level down to checkInvalidReadDuringRender/checkInvalidWriteDuringRender. There are more complex scenarios than this one in the library that we would like to address as well.