Skip to content

Conversation

@vmoroz
Copy link
Member

@vmoroz vmoroz commented May 30, 2020

In the Microsoft.ReactNative.IntegratonTests we already had a test for the ReactNativeHost, but it was not running because we have to fix a number of issues. In this PR we are addressing these issue and make the test to run.
To enable the tests the following changes had to be done:

  • Expose ReactNativeHost API for instance load and unload that return IAsyncAction.
  • Created an adapter from Mso::Future to IAsyncAction.
  • Implement Application::Current() property override to return nullptr instead of throwing exceptions if the current application is absent.
  • Changed native modules that depend on the Application::Current() to check for nullptr and do nothing in such case.
  • Added collection of IReactPackageProvider to ReactInstanceSettings.
  • Queue up the CallJSFunction calls while JavaScript is loaded. Otherwise, native modules, that are created while loading JS, lose the call.
  • Fixed sample applications which are not working in current code.

Fixes #5057
Fixes #5070

Microsoft Reviewers: Open in CodeFlow

@vmoroz vmoroz requested review from a team, acoates-ms, aeulitz and asklar May 30, 2020 23:22
@vmoroz vmoroz requested a review from a team as a code owner May 30, 2020 23:22
/// <param name="e">Details about the launch request and process.</param>
void App::OnLaunched(LaunchActivatedEventArgs const &e) {
super::OnLaunched(e);
Frame rootFrame = Window::Current().Content().as<Frame>();
Copy link
Member

@dannyvv dannyvv May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just observing: Aleksander's PR has window.Activate(); before navigating...
#Resolved

Copy link
Member

@asklar asklar May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just seeing this now. It's not necessary to call Window::Activate if you call super::OnLaunched. Otherwise you must call it. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works without it. Like Alexander says it is redundant in this context.


In reply to: 432967013 [](ancestors = 432967013)

@@ -1,3 +1,5 @@
<!-- Copyright (c) Microsoft Corporation. All rights reserved.
Licensed under the MIT License. -->
Copy link
Member

@dannyvv dannyvv May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[](start = 36, length = 2)

All Xml comments end with double-space...
Or is that the auto-formatter? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was copy & paste. I can fix it.


In reply to: 432967124 [](ancestors = 432967124)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed extra space.


In reply to: 433240152 [](ancestors = 433240152,432967124)

return ReloadInstance();
}

IAsyncAction ReactNativeHost::ReloadInstance() noexcept {
Copy link
Member

@dannyvv dannyvv May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me this seems a bit counter-intuitive...
To me it seems backwards. LoadInstance and UnloadInstance would have implemenataton. and ReloadInstance would just call UnloadInstance and then LoadInstance.

I realize it is an artifact of the m_reactHost api... just calling out if feels 'funny'.
#Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ReloadInstance is much more powerful than LoadInstance because otherwise requiring to always unload before load would be too difficult and error prone. I would rather have it as the prime API instead of inventing what to do if developers call LoadInstance twice without calling UnloadInstance between them. Should we crash or ignore? Plus, the Reload call could let us optimize some internals if needed.
So, why to have LoadInstance in addition to the ReloadInstance which does the same work? The answer is the "syntactic sugar". The ReloadInstance call looks strange when we just start application. Saying LoadInstance seems to be more logical. Internally the 'unload' part is noop in that case anyway.


In reply to: 432967474 [](ancestors = 432967474)

host.InstanceSettings().UseDeveloperSupport(false);
host.InstanceSettings().UseWebDebugger(false);
host.InstanceSettings().UseFastRefresh(false);
host.InstanceSettings().UseLiveReload(false);
Copy link
Member

@dannyvv dannyvv May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've noticed that also in the template these are all set at the same time, should we have a single method to toggle these settings (in case we get more in the future) i.e.
UseDevSettings(true|false) #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a good point. We could implement it in a separate PR.


In reply to: 432968392 [](ancestors = 432968392)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have created Issue #5078 to address it in the future.


In reply to: 433245285 [](ancestors = 433245285,432968392)


namespace Mso {

struct AsyncActionFutureAdapter : winrt::implements<
Copy link
Member

@dannyvv dannyvv May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AsyncActionFutureAdapter [](start = 7, length = 24)

Just checking if we have a convention like is ocmmon in C# to have one type per file and match the filename to the type.
Here I see fileName: futureWinRT.h, with type struct AsyncActionFutureAdapter #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose the file name for the 'growth'. In C++ we often have multiple types in the same file.
The WinRT has four async interfaces based on the IAsyncInfo: IAsyncAction, IAsyncOperation, IAsyncActionWithProgress, an IAsyncOperationWithProgress. This file must contain conversion to all four of them and then conversion back from them. I just did a required bare minimum at this point.


In reply to: 432968554 [](ancestors = 432968554)

Copy link
Member

@dannyvv dannyvv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

#pragma once

#ifndef WIN32_LEAN_AND_MEAN
#define WIN32_LEAN_AND_MEAN
Copy link
Contributor

@NickGerleman NickGerleman May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI we will already define these for all projects including React.cpp.props. No harm in adding this to the PCH as well though. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove it. It is a not related left-over from solving a build error.


In reply to: 432987546 [](ancestors = 432987546)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed extra defs from pch.h


In reply to: 433248481 [](ancestors = 433248481,432987546)

namespace winrt::impl {

template <>
WINRT_IMPL_AUTO(Windows::UI::Xaml::Application)
Copy link
Contributor

@NickGerleman NickGerleman May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define our own helper instead of replacing the original? This feels a bit fragile, or unexpected. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think in this case having a helper would make more sense. I was just played too much with the idea how I can augment the generated consumer classes with custom code instead of having all these manually written ReactContext, ReactDispatcher, and other classes.


In reply to: 432988186 [](ancestors = 432988186)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defined TryGetCurrentApplication instead.


In reply to: 433249993 [](ancestors = 433249993,432988186)

{
std::scoped_lock lock{m_mutex};
if (m_state == ReactInstanceState::HasError || m_state == ReactInstanceState::Unloaded) {
jsCallQueue = std::move(m_jsCallQueue);
Copy link
Contributor

@NickGerleman NickGerleman May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not clear()? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid calling destructors under the lock. It is a common technique. I will add a comment.


In reply to: 432988947 [](ancestors = 432988947)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added comments


In reply to: 433251387 [](ancestors = 433251387,432988947)

runtimeclass ReactNativeHost {
ReactNativeHost();

IVector<IReactPackageProvider> PackageProviders { get; set; };
Copy link
Contributor

@NickGerleman NickGerleman May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was previous behavior of someone set this? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was buggy. It was like giving external code ability to access private variables. As a rule of thumb we must not allow the collections to be set, but rather just modify them. Why? For example, if we decide that the internal storage must be thread-safe (which is not today), then there is no way for us to enforce it if we allow the collection to be set. It means that the API has an unnecessary 'freedom' that puts us in the corner when we want to improve the implementation.


In reply to: 432989273 [](ancestors = 432989273)

try {
winrt::slim_lock_guard const guard(m_lock);
RethrowIfFailed();
return 0;
Copy link
Contributor

@NickGerleman NickGerleman May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: S_OK? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The C++/WinRT always uses 0 for some reason.


In reply to: 432989753 [](ancestors = 432989753)

}

uint32_t Id() const noexcept {
return 1;
Copy link
Contributor

@NickGerleman NickGerleman May 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the contract for this allow collisions? Any reason we do this instead of a monontonically increasing int? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what the C++/WinRT uses internally. It seems that the Id property is never used. The code is 'adapted' from their implementation of IAsyncAction for co-routines.


In reply to: 432990053 [](ancestors = 432990053)

@asklar
Copy link
Member

asklar commented May 31, 2020

In the Microsoft.ReactNative.IntegratonTests we already had a test for the ReactNativeHost, but it was not running because we have to fix a number of issues. In this PR we are addressing these issue and make the test to run.
To enable the tests the following changes had to be done:

  • Expose ReactNativeHost API for instance load and unload that return IAsyncAction.
  • Created an adapter from Mso::Future to IAsyncAction.
  • Implement Application::Current() property override to return nullptr instead of throwing exceptions if the current application is absent.
  • Changed native modules that depend on the Application::Current() to check for nullptr and do nothing in such case.
  • Added collection of IReactPackageProvider to ReactInstanceSettings.
  • Queue up the CallJSFunction calls while JavaScript is loaded. Otherwise, native modules, that are created while loading JS, lose the call.
  • Fixed sample applications which are not working in current code.

Fixes #5057
Fixes #5070

Microsoft Reviewers: Open in CodeFlow

I see there are some changes to instance load state, will this change fix #5002 too? #Resolved

@vmoroz
Copy link
Member Author

vmoroz commented Jun 1, 2020

No, this change does not fix #5002, but it gets us closer. As we write more and more tests that exercise the ReactNativeHost API, we can start provide guarantees that it behaves correctly. For example, even the simplest test has shown that we had an issue implementing the CallJSFunction. The issue #5002 indicates that there is something wrong with the ReactViewHost handling. I hope that we can address it by adding more tests.


In reply to: 636544322 [](ancestors = 636544322)

@vmoroz
Copy link
Member Author

vmoroz commented Jun 1, 2020

@JunielKatarn, @acoates-ms, I am disabling a number of Desktop unit tests that prevent many of PRs successful checks. None of these tests can run locally in Visual Studio. If we need them, then they must be fixed/updated to be run successfully locally in Visual Studio first before we require them to pass in CI loop.

@asklar
Copy link
Member

asklar commented Jun 1, 2020

@JunielKatarn, @acoates-ms, I am disabling a number of Desktop unit tests that prevent many of PRs successful checks. None of these tests can run locally in Visual Studio. If we need them, then they must be fixed/updated to be run successfully locally in Visual Studio first before we require them to pass in CI loop.

@vmoroz I'm actually able to run these tests locally which is why I was trying to figure out why they're failing in the pipeline. I'm assuming these fail for you? are you starting the bundler in \vnext ?

@@ -1,3 +1,5 @@
<!-- Copyright (c) Microsoft Corporation. All rights reserved.
Copy link

@NikoAri NikoAri Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All rights reserved. [](start = 42, length = 20)

From:
https://docs.opensource.microsoft.com/changelog.html?q=all%20rights%20reserved

CELA open source policy
The term All Rights Reserved is no longer recommended to be in MIT license copyright headers. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is addressed by PR #5083


In reply to: 433393589 [](ancestors = 433393589)

var frame = Window.Current.Content as Frame;
frame.Navigate(typeof(MainPage));
Window.Current.Activate();
var frame = (Frame)Window.Current.Content;
Copy link
Member Author

@vmoroz vmoroz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Frame)Window.Current.Content; [](start = 24, length = 30)

Note that in C# the 'as' conversion returns null if conversion failed, while the C-style conversion throws an exception. It is different from the C++/WinRT where the 'as' conversion throws an exception while 'try_as' returns null in case of failure.
This is why the code here is changed here to use C-style conversion to match the C++/WinRT 'as()' function call which is a correct functionality since we do not check for null after the call. #Resolved

@vmoroz vmoroz added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Jun 1, 2020
@ghost
Copy link

ghost commented Jun 1, 2020

Hello @vmoroz!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (@msftbot) and give me an instruction to get started! Learn more here.


TEST_METHOD(RequestGetSucceeds) {
// This test always fails because the requested resource does not exist.
// TEST_METHOD(RequestGetSucceeds) {
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// TEST_METHOD [](start = 2, length = 14)

Does whatever harness this project uses support a SKIPTESTMETHOD annotation? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is VS test. We cannot use the SKIPTESTMETHOD here.


In reply to: 433405820 [](ancestors = 433405820)

} // namespace Microsoft::VisualStudio::CppUnitTestFramework

// None of these tests are runnable
#if 0
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove/deactivate TEST_CLASS attribute, but keep it compiling? #Pending

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for the other tests in the folder.


In reply to: 433406671 [](ancestors = 433406671)

<Filter>Modules</Filter>
</ClCompile>
<ClCompile Include="..\ReactUWP\Modules\DeviceInfoModule.cpp">
<Filter>Modules</Filter>
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modules [](start = 6, length = 24)

The PR doesn't contain a corresponding change to the .vcxproj. Is it not needed? #Resolved

Copy link
Member Author

@vmoroz vmoroz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that VS has a bug managing the filters file. The file was removed by another PR, but when someone opens the project we see the filter file changed. I do not know a good way to avoid it unless VS fixes the issue. #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is done by some other change. VS has a bug that filters file is not updated/saved correctly. When next developer opens VS project they see such random changes. No harm to bring this change along.


In reply to: 433407937 [](ancestors = 433407937)

ReactInstanceSettings InstanceSettings { get; set; };

void ReloadInstance();
Windows.Foundation.IAsyncAction LoadInstance();
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows.Foundation.IAsyncAction LoadInstance(); [](start = 4, length = 47)

Can we please add doc-style comments to document intended usage of the API? #Pending

WaitingForDebugger,
Loaded,
HasError,
Unloaded,
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please add comments detailing how these states relate to which operations on what API components? #Pending

@ghost ghost merged commit 80d1a40 into microsoft:master Jun 1, 2020
<ClInclude Include="$(MSBuildThisFileDirectory)UI.Input.h" />
<ClInclude Include="$(MSBuildThisFileDirectory)UI.Popups.h" />
<ClInclude Include="$(MSBuildThisFileDirectory)UI.Text.h" />
<ClInclude Include="$(MSBuildThisFileDirectory)XamlHelpers.h" />
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

)XamlHelpers.h [](start = 50, length = 14)

Is this intentional? The change appears to be adding a XamlUtils.h file. #Pending

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it is another artifact of failed .filters file handling...


In reply to: 433448516 [](ancestors = 433448516)

#include <winrt/Windows.UI.Xaml.h>

#define XAML_CPPWINRT_NAMESPACE winrt::Windows::UI::Xaml
namespace xaml = winrt::Windows::UI::Xaml;
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my education: Is this header file specifically meant for Xaml-based consumers? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is for XAML users. We are going to split up the project to separate the "core" files from XAML. We just need to change some ABI files first.


In reply to: 433449630 [](ancestors = 433449630)

// app into a UWP app). In the long term, the issue should get addressed by splitting the Microsoft.ReactNative.dll
// into a "core" and a "UI" DLL, and by having the tests in this project target the "core" DLL.
SKIPTESTMETHOD(JsFunctionCall_Succeeds) {
TEST_METHOD(JsFunctionCall_Succeeds) {
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TEST_METHOD [](start = 1, length = 12)

Awesome! #Resolved

#else
std::string bundlePath = m_devSettings->bundleRootPath + jsBundleRelativePath + ".bundle";
std::string bundlePath =
(fs::path(m_devSettings->bundleRootPath) / (jsBundleRelativePath + ".bundle")).u8string();
Copy link
Contributor

@aeulitz aeulitz Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ [](start = 51, length = 1)

Is this an operator overload for FS paths? #Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is how the std::filesystem concatenates paths.


In reply to: 433453526 [](ancestors = 433453526)


// None of these tests are runnable
#if 0
TEST_CLASS (WebSocketJSExecutorIntegrationTest) {
Copy link
Contributor

@JunielKatarn JunielKatarn Jun 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wish I had seen this review before.
There are more orthodox ways to disable these tests.
We should aim to run them in UWP too, actually.

#Resolved

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss it offline.


In reply to: 434168343 [](ancestors = 434168343)

@NickGerleman
Copy link
Contributor

Removing the Backport to 0.62 label, since we need to rename it as part of the deprecation in favor of Request Backport which was already added to this PR. Triage of this item is happening in the next round it sounds like.

@Khalef1
Copy link
Contributor

Khalef1 commented Jun 8, 2020

We discussed this in triage. Full change is too large to be backported, but we should backport the CallJSFunction queue only

@Khalef1
Copy link
Contributor

Khalef1 commented Jun 8, 2020

Please note that only part of this change has been approved for backport

acoates-ms added a commit that referenced this pull request Jun 12, 2020
* [0.62] Cherry pick PR #5071 for CallJsFunction queuing

* Change files

Co-authored-by: Andrew Coates <30809111+acoates-ms@users.noreply.github.com>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sample applications do not run after PR #4949 Queue up CallJSFunction while bundle is loaded

8 participants