-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Hermes Inspector #7322
Hermes Inspector #7322
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leaving a batch of comments
} | ||
#endif | ||
|
||
// Add js engine information to Error.prototype so in error reporting we |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious of where this pattern comes from. I know some built-in RN code relies on testing the presence of "global.HermesInternal" for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/facebook/react-native/blob/master/Libraries/Core/ExceptionsManager.js#L76 is using this .. Yeah, it would be nice to have a more generic mechanism ..
vnext/Shared/DevSupportManager.cpp
Outdated
@@ -232,6 +235,28 @@ void DevSupportManager::StopPollingLiveReload() { | |||
m_cancellation_token = true; | |||
} | |||
|
|||
void DevSupportManager::startInspector(const std::string &packagerHost, const uint16_t packagerPort) { | |||
#if defined(HERMES_ENABLE_DEBUGGER) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the motivation for adding a preprocessor definition to disable this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes more sense to disable based on USE_HERMES here ... This inspector start/stop doesn't make sense for other engines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've reverted back to using HERMES_ENABLE_DEBUGGER .. We don;t always have the debugger available .. for e.g. in release flavor ... We know/control whether the debugger/inspector is included using this flag.
m_packagerWebSocketConnection = | ||
std::make_shared<Microsoft::React::WinRTWebSocketResource>(std::move(url), std::move(certExceptions)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WinRTWebSocketResource
is an abstraction over MessageWebSocket
created as an adapter for a Reka service/native module. Consider using MessageWebSocket
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Nick .. Makes sense .. But, WinRTWebSocketResource works fine for now and make this work item more manageable :) .. It will be a lot of work to rewrite the code and .. I'd prefer doing the refactoring as another change after this gets in ..
folly::dynamic response = InspectorProtocol::getMessageForPackager( | ||
InspectorProtocol::EventType::WrappedEvent, | ||
InspectorProtocol::getVMResponseForPackagerPayload(m_pageId, std::move(message))); | ||
std::string responsestr = folly::toJson(response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using Windows::Data::Json
instead of folly::dynamic for JSON manipulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thoughts/preference here is just like the Websocket change that you suggested above.. I don't see anything wrong with folly::json for the usage here .. The lower level inspector implementation already uses folly_json anyways ..
I understand that we should try to limit follly dependencies hoping someday we could get rid of it .. but i think it is not practical in near term ..
I definitely want to rewrite using Windows::Data::Json, but i don't want to invest the time to rewrite the code and test it all over again. Hence, would like to defer it to another change after this PR gets in. It is alive for quite some time !
Hope you agree.
std::make_shared<Microsoft::React::WinRTWebSocketResource>(std::move(url), std::move(certExceptions)); | ||
|
||
m_packagerWebSocketConnection->SetOnError([this](const Microsoft::React::IWebSocketResource::Error &err) { | ||
assert(false && "Websocket connection failed !"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there legitimate cases where we can see network errors? E.g. will this cause us to assert if we start a session with direct debugging enabled, but don't have Metro open?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.. Yeah .. we shouldn't assert i think here .. Changing to a trace log ..
@@ -0,0 +1,461 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to point out the changes that were made compared to the original?
The full size of the minimal future implementation still seems quite large. I worry a bit about how we can keep this up to date (e.g. if the folly headers this relies on changes their API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned about the size and upkeep too; I would love to see the changes upstream before we consume them here. It would be hard to guarantee that our custom stuff doesn't get broken as changes upstream flow in, at least if we are able to upstream it we can guarantee that breaks are caught in the folly repo the minute they happen
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for sharing these vital concerns .. Yes, the current "min" implementation is not really minimal.. and it's risky for it have dependency on folly headers .. Let me try to make the minfuture implementation standalone and truly minimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer have the minimal implementation .. Even in the hermes build scripts that we use, we don't have any custom implementation, instead we use the old folly code .. All the old folly code is contained within the dll public api surface ..
c4da5fe
to
1e7c6b2
Compare
vnext/PropertySheets/JSEngine.props
Outdated
@@ -2,8 +2,8 @@ | |||
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
|
|||
<PropertyGroup> | |||
<UseHermes Condition="'$(UseHermes)' == ''">false</UseHermes> | |||
<HermesVersion Condition="'$(HermesVersion)' == ''">0.7.2-microsoft.5</HermesVersion> | |||
<UseHermes Condition="'$(UseHermes)' == ''">true</UseHermes> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like it is turning Hermes on by default, do we have a writeup of what is working/what's missing, is the tooling ready, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll work on a writeup ..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change to globally enable hermes was unintentional .. Reverted.
std::string packageName = winrt::to_string(winrt::Windows::ApplicationModel::Package::Current().DisplayName()); | ||
|
||
std::string deviceName("RNWHost"); | ||
auto hostNames = winrt::Windows::Networking::Connectivity::NetworkInformation::GetHostNames(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this require new Capabilities declarations on the appx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the current state, these values can be anything..
On Android, there is code which relies on these values : https://github.com/facebook/react-native/blob/ca499a61971de30695b7834fb5a16e09bcfb6074/ReactCommon/hermes/inspector/chrome/AutoAttachUtils.cpp#L40
Essentially, on POSIX system there is a special way to enable "pause on load" .. On Windows, we don't have that.. On windows, the "pause on load" will be enabled on refresh.
I would love to understand what the debugging experience is after this change. Do I connect VS in script mode? or? |
Alex, I'll have a doc very soon .. I'd like to get this change in before the doc though. Post this change, you can navigate to "chrome://inspect (or edge://inspect)" .. add "8081" as a watch port .. and the attach links should pop up .. Clicking on the links will pop up the inspector window .. you can press "Ctrl+F" to pick a source file and add break point .. And the heap profiler tools will also work .. CPU profilers won't work .. Debugging from VSCode should mostly work just like how it work for Android now .. All our endpoints and protocols are identical .. |
…me formatting fixes
vnext/Desktop.UnitTests/React.Windows.Desktop.UnitTests.vcxproj
Outdated
Show resolved
Hide resolved
@@ -21,6 +21,11 @@ | |||
#include <winrt/Windows.Web.Http.Headers.h> | |||
#include <winrt/Windows.Web.Http.h> | |||
|
|||
#ifdef HERMES_ENABLE_DEBUGGER | |||
#include <winrt/Windows.ApplicationModel.Activation.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like you only use Windows.ApplicationModel below, i.e. no classes in the Activation namespace, is that right? if so, you can remove this include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this include, the compiler says: error C3779: 'winrt::Windows::ApplicationModel::Package::Current': a function that returns 'auto' cannot be used before it is defined
class HermesExecutorRuntimeAdapter : public facebook::hermes::inspector::RuntimeAdapter { | ||
public: | ||
HermesExecutorRuntimeAdapter( | ||
std::shared_ptr<jsi::Runtime> runtime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
facebook::hermes::HermesRuntime &hermesRuntimeRef = *hermesRuntime; | ||
|
||
m_runtime = std::move(hermesRuntime); | ||
m_own_thread_id = std::this_thread::get_id(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JS queue is necessarily bound to a single thread. I wonder if we can avoid binding to a specific thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are now building inspector into a standalone dll, named "hermesinspector.dll", as part of the hermes build pipelines. It simplifies the RNW integration. "hermesinspector.dll" exposes the inspector APIs which are called from the websocket client implementation in the RNW repo.
The new build scripts to build hermesisnpector.dll in hermes-windows repo are kept here : https://github.com/microsoft/hermes-windows/tree/v0.7.2-microsoft/.ado/scripts . Essentially, we are building the hermes inspector code in react-windows, folly future implementation from the old version of folly which is still consumed by the open source android builds of react-native, along with the other dependencies such as boost, glog/dconversion stubs, into the new dll.
Microsoft Reviewers: Open in CodeFlow