Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"type": "prerelease",
"comment": "Removed the ref count work around from classes derived from ReactApplications",
"packageName": "react-native-windows",
"email": "vmorozov@microsoft.com",
"dependentChangeType": "patch",
"date": "2020-04-30T15:22:39.524Z"
}
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,6 @@ App::App() noexcept {
PackageProviders().Append(winrt::SampleLibraryCS::ReactPackageProvider());

InitializeComponent();

// This works around a cpp/winrt bug with composable/aggregable types tracked
// by 22116519
AddRef();
m_inner.as<::IUnknown>()->Release();
}

} // namespace winrt::SampleAppCpp::implementation
14 changes: 12 additions & 2 deletions vnext/Microsoft.ReactNative/ReactApplication.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "pch.h"
#include "ReactApplication.h"
#include "ReactApplication.g.cpp"

#include "Modules/LinkingManagerModule.h"
#include "ReactNativeHost.h"

Expand All @@ -22,7 +23,16 @@ using namespace xaml::Navigation;

namespace winrt::Microsoft::ReactNative::implementation {

ReactApplication::ReactApplication() noexcept {
ReactApplication::ReactApplication() = default;

ReactApplication::ReactApplication(IInspectable const &outer) noexcept : ReactApplication{} {
// The factory is usually called in the base generated class. We call it here to pass correct
// 'outer' interface to enable inheritance from the ReactApplication class in user code.
impl::call_factory<xaml::Application, xaml::IApplicationFactory>([&](xaml::IApplicationFactory const &f) {
[[maybe_unused]] auto winrt_impl_discarded =
f.CreateInstance(outer ? outer : static_cast<IInspectable const &>(*this), this->m_inner);
});

Suspending({this, &ReactApplication::OnSuspending});

#if defined _DEBUG && !defined DISABLE_XAML_GENERATED_BREAK_ON_UNHANDLED_EXCEPTION
Expand Down Expand Up @@ -111,7 +121,7 @@ void ReactApplication::OnActivated(IActivatedEventArgs const &e) {
}

void ReactApplication::OnLaunched(LaunchActivatedEventArgs const &e) {
Super::OnLaunched(e);
base_type::OnLaunched(e);
// auto args = std::wstring(e.Arguments().c_str());
this->OnCreate(e);
}
Expand Down
55 changes: 51 additions & 4 deletions vnext/Microsoft.ReactNative/ReactApplication.h
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,55 @@
#pragma once

#include "ReactApplication.g.h"
#include <CppWinRTIncludes.h>
#include "ReactNativeHost.h"

namespace winrt::Microsoft::ReactNative::implementation {

struct ReactApplication : ReactApplicationT<ReactApplication> {
using Super = ReactApplicationT<ReactApplication>;
// NoDefaultCtorReactApplication_base is a copy of the generated ReactApplication_base
// without the default constructor where it calls a factory for the base type.
// This is done to fix the aggregation issue in types inheriting from the ReactApplication.
// We call the factory in the ReactApplication constructor where we can pass correct
// 'outer' interface.
//
// This class must match the generated ReactApplication_base.
// It must be updated if the shape of generated ReactApplication_base is changed in future.
// The only difference is that this class has no default constructor.
template <typename D, typename... I>
struct __declspec(empty_bases) NoDefaultCtorReactApplication_base
: implements<
D,
Microsoft::ReactNative::ReactApplication,
composable,
composing,
xaml::IApplicationOverrides,
xaml::IApplicationOverrides2,
I...>,
impl::require<D, xaml::IApplication, xaml::IApplication2, xaml::IApplication3>,
impl::base<D, xaml::Application>,
xaml::IApplicationOverridesT<D>,
xaml::IApplicationOverrides2T<D> {
using base_type = NoDefaultCtorReactApplication_base;
using class_type = Microsoft::ReactNative::ReactApplication;
using implements_type = typename NoDefaultCtorReactApplication_base::implements_type;
using implements_type::implements_type;
Copy link

Choose a reason for hiding this comment

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

using implements_type::implements_type; [](start = 2, length = 39)

(nit) what does this do?

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 brings constructors from the implements_type base class. I am not sure why it is need. I effectively copy&pasted the generated code and did minimal changes to it: deleted default constructor.


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

using composable_base = xaml::Application;

hstring GetRuntimeClassName() const {
return L"Microsoft.ReactNative.ReactApplication";
}

protected:
using dispatch = impl::dispatch_to_overridable<D, xaml::IApplicationOverrides, xaml::IApplicationOverrides2>;
auto overridable() noexcept {
return dispatch::overridable(static_cast<D &>(*this));
}
};

struct ReactApplication : NoDefaultCtorReactApplication_base<ReactApplication> {
Copy link
Member

Choose a reason for hiding this comment

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

Why did the base type for this need to change? Was there something provided in the generated code that was conflicting with this override?

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 default constructor of the ReactApplicationT calls the factory for the XAML Application class. If I do not replace the base class, then the XAML Application is going to be created twice: one in the base default constructor and the second in the our constructor with the outer reference.

Ideally, the generated class must have two constructors or one with an optional outer parameter.


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

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Yes that makes sense.

public: // ReactApplication ABI API
ReactApplication() noexcept;
ReactApplication();
ReactApplication(IInspectable const &outer) noexcept;

ReactNative::ReactInstanceSettings InstanceSettings() noexcept;
void InstanceSettings(ReactNative::ReactInstanceSettings const &value) noexcept;
Expand Down Expand Up @@ -57,6 +97,13 @@ struct ReactApplication : ReactApplicationT<ReactApplication> {

namespace winrt::Microsoft::ReactNative::factory_implementation {

struct ReactApplication : ReactApplicationT<ReactApplication, implementation::ReactApplication> {};
// Override the CreateInstance method to pass baseInterface to the ReactApplication constructor
// to support correct COM aggregation that is need to inherit from the ReactApplication.
struct ReactApplication : ReactApplicationT<ReactApplication, implementation::ReactApplication> {
auto CreateInstance(IInspectable const &baseInterface, IInspectable &innerInterface) {
return impl::composable_factory<implementation::ReactApplication>::template CreateInstance<
Microsoft::ReactNative::ReactApplication>(baseInterface, innerInterface, baseInterface);
}
};

} // namespace winrt::Microsoft::ReactNative::factory_implementation
5 changes: 0 additions & 5 deletions vnext/local-cli/generator-windows/templates/cpp/src/App.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,6 @@ App::App() noexcept
REACT_REGISTER_NATIVE_MODULE_PACKAGES(); //code-gen macro from autolink

InitializeComponent();

// This works around a cpp/winrt bug with composable/aggregable types tracked
// by 22116519
AddRef();
m_inner.as<::IUnknown>()->Release();
}