Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implemented ReactHost for Microsoft.ReactNative #4009

Closed
wants to merge 5 commits into from
Closed

Implemented ReactHost for Microsoft.ReactNative #4009

wants to merge 5 commits into from

Conversation

vmoroz
Copy link
Member

@vmoroz vmoroz commented Jan 31, 2020 •
edited
Loading

This PR is replaced with #4022 because its diff shows files that were previously changed by different PRs and picked up after merge with master.

This is an initial set of changes to implement ReactHost for Microsoft.ReactNative.
More details are here: react-native-community/discussions-and-proposals#183

The code is based on what we currently have in Microsoft Office and we still need to do some clean up.

Why the change?

  • We want the ReactInstance to be instantiated in a background thread before we draw any UI.
  • We want to support multiple RootViews per ReactInstance.
  • We want to have threads working in coordinated way without thread blocking.

What works so far?

  • The Sample apps work in Debug mode with Web debugging.
  • The Sample apps work with bundles.
  • The ReactUWP should continue to work. We have built the UwpReactInstanceProxy to enable all components that used the 'legacy' react instance API to continue to work with the new code. Minimal changes are done to ReactUWP code.

Future work?

  • After the initial submission we are going to iterate on removing extra layers between Microsoft.ReactNative and FB react-native code, most notably the OInstance.cpp and other ReactInstance wrappers.
  • Improve the Microsoft.ReactNative ABI-safe API.
  • Change namespaces and some type names.
  • Change view managers and native modules to use the new APIs.
  • Changing ReactUWP to be a layer on top of the new code and provide other facilities to enable transition of existing code to the new API.
Microsoft Reviewers: Open in CodeFlow

@vmoroz vmoroz requested a review from a team as a code owner January 31, 2020 18:11
@ghost ghost added the vnext label Jan 31, 2020
jonthysell
jonthysell requested changes Feb 3, 2020
View reviewed changes
packages/microsoft-reactnative-sampleapps/windows/SampleAppCPP/App.cpp Outdated
@@ -16,15 +16,14 @@ namespace winrt::SampleAppCpp::implementation {
/// WinMain().
/// </summary>
App::App() noexcept {
MainComponentName(L"SampleApp");
JavaScriptMainModuleName(L"index.windows");
InstanceSettings().MainComponentName(L"SampleApp");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to change? I think if you're using the easy-mode "ReactApplicaton", we should maintain these properties as pass-throughs to the properties on the host.

Doin this should also fix the CI build errors you're seeing.

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 idea was to simplify theReactApplication API. I have restored these properties for now.

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

@ghost ghost added Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) and removed Needs: Author Feedback The issue/PR needs activity from its author (label drives bot activity) labels Feb 3, 2020
@vmoroz vmoroz mentioned this pull request Feb 4, 2020
Closed
@vmoroz vmoroz changed the title Implemented ReactHost for Microsoft.ReactNative[PREVIEW - DO NOT MERGE YET] Implemented ReactHost for Microsoft.ReactNative Feb 4, 2020
@vmoroz vmoroz added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Feb 4, 2020
@ghost
Copy link

ghost commented Feb 4, 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.

@vmoroz vmoroz removed the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Feb 4, 2020
@vmoroz vmoroz mentioned this pull request Feb 4, 2020
Merged
@vmoroz
Copy link
Member Author

vmoroz commented Feb 4, 2020

This PR is replaced with #4022 because its diff shows files that were previously changed by different PRs and picked up after merge with master.

@vmoroz vmoroz closed this Feb 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonthysell jonthysell jonthysell approved these changes

@RobMeyer RobMeyer Awaiting requested review from RobMeyer

@stmoy stmoy Awaiting requested review from stmoy

@acoates-ms acoates-ms Awaiting requested review from acoates-ms

@ddalp ddalp Awaiting requested review from ddalp

@NikoAri NikoAri Awaiting requested review from NikoAri

@stecrain stecrain Awaiting requested review from stecrain

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vmoroz @jonthysell