-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Hitting 'r' in the packager should reload the instance #4665
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
Conversation
| auto response = | ||
| std::string(Microsoft::Common::Utilities::CheckedReinterpretCast<char *>(data.data()), data.size()); | ||
| auto json = | ||
| winrt::Windows::Data::Json::JsonObject::Parse(Microsoft::Common::Unicode::Utf8ToUtf16(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.
could use folly::parseJSON here instead but this is fine too
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.
Trying to avoid adding extra folly usage, since long term once we are off the legacy bridge, we might be able to ditch folly altogether.
|
Hello @acoates-ms! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
| DownloadFromAsync(facebook::react::DevServerHelper::get_LaunchDevToolsCommandUrl(settings.debugHost)).get(); | ||
| } | ||
|
|
||
| std::future<void> DevSupportManager::CreatePackagerConnection(const facebook::react::DevSettings &settings) { |
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.
std::future [](start = 0, length = 17)
Is it possible to avoid using the std::future? It does not offer a continuation. The only way to observe it is to block a thread which is an anti-pattern.
It is better to use either IAsyncAction or Mso::Future.
| #ifdef DEFAULT_CPPWINRT_EXCEPTIONS | ||
| co_await async; | ||
| #else | ||
| co_await lessthrow_await_adapter<winrt::Windows::Foundation::IAsyncAction>{async}; |
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 does this do?
- Any reason this is using std::future instead of winrt::fire_and_forget as some other code does?
|
|
||
| try { | ||
| LaunchDevTools(settings); | ||
| CreatePackagerConnection(settings); |
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.
CreatePackagerConnection [](start = 4, length = 24)
Why do we ignore the result?
Would it be better to have a sequence of async actions uisng .Then or co_await?
The packager implements a websocket that allows apps to be recieve commands from the packager. Two obvious commands are 'reload' and 'show devmenu'.
This PR makes the instance connect to this websocket and see the commands. It hooks up reload. The dev menu command is left as a todo for now, as the dev menu is currently tied to the root view, which is probably incorrect and will require some more refactoring.
Microsoft Reviewers: Open in CodeFlow