microsoft / v8-jsi

React Native V8 JSI adapter
Other
157 stars 36 forks source link

Remove JSI JSError::setStack method #125

Closed vmoroz closed 2 years ago

vmoroz commented 2 years ago

The issue

The JSI JSError::setStack method is a custom extension to JSI API. It is required to record V8 error stack. Since it is better to keep JSI public files to be the same as they are defined in Hermes and ReactNative, we need to find a way to remove it.

Solution

In this PR we remove JSError::setStack method by using special private field accessors for the jsi::JSError class. It is based on the technique described in this article: http://bloglitb.blogspot.com/2010/07/access-to-private-members-thats-easy.html

Microsoft Reviewers: Open in CodeFlow
tudorms commented 2 years ago

JSError(Runtime& rt, std::string message, std::string stack);

I think we can change the implementation of this constructor override in jsi.cpp to do what we want instead (it's not used anywhere else in v8jsi).

That way, jsi.h is unchanged, thus the public ABI of JSError is compatible with public jsi.h headers.

The alternative (to keep jsi.h / jsi.cpp completely unchanged) is to inject a small JavaScript piece into every runtime that creates a custom "Error" type deriving from Error with a factory maker for it, and using the JSError constructor that takes a Value&& in C++; don't know if it's worth the effort though.

class V8JsiError extends Error { constructor(message, stack) { super(message); this.name = 'V8JsiError'; this.stack = stack; } }

We can also try upstreaming a change to Hermes (current JSI source of truth), although that takes 6+ months to propagate.


Refers to: src/jsi/jsi.h:1297 in 8ce43a0. [](commit_id = 8ce43a0115a8bbba9454b84d0d594f09b824e502, deletion_comment = False)

vmoroz commented 2 years ago

This approach requires initialization of static variables in template instantiation. For some reason it does not work in the v8jsi.dll. I am closing this PR to avoid any accidental merge until I get a better solution.