Implement String ViewManager Command IDs #4667

NickGerleman · 2020-04-21T22:32:15Z

In 0.61 Facebook added the ability to dispatch view manager commands using a string directly instead of the current indirection of getting a command ID from the view manager. This is leveraged in 0.62 in a couple places, including for Switches. UIManager overloads for string commands are added.

This change impacts the C++ Microsoft.ReactNative ViewManager APIs C# and upstream view managers abstract away command IDs where they are currently left explicit in our C++ ViewManagers. Given our desire to implement built-in controls, we will need to support string commands here as well. This is made trickier with Facebook still publicly documenting grabbing command IDs from the VIewManager.

Providing parallel explicit DispatchCommand functions to Microsoft.ReactNative ViewManagers would be too confusing to our users. Instead of adding parallel paths to the C++ ViewManager for commands without IDs, we remove the ID indirection entirely. Instead of internal ID mapping like we previously did with the C# ViewManagers, we now map constants to literal strings of themselves. This allows ViewManagers to accept the new form of literal string commands dispatched from JS without breaking the documented workflow of getting command IDs from the ViewManager.

Eventually we should be able to remove integer only command IDs, as they are internally deprecated.

Validated C# and C++ commands still work through the sample app. Validated switch behavior using the RNTester page.

Fixes #4596

Microsoft Reviewers: Open in CodeFlow

NickGerleman · 2020-04-21T22:33:07Z

packages/E2ETest/app/ControlStyleTestPage.tsx

-          placeholder='TextBox'
-          placeholderTextColor='rgba(225,225,225,0.7)'
-          editable={false} />
+        <Switch


This file got reformatted quite a bit since CI hasn't been enforcing our eslint file for this project. We should probably fix this, because VS Code will pick the file up and do it for you on save.

NickGerleman · 2020-04-21T22:34:04Z

packages/microsoft-reactnative-sampleapps/windows/SampleAppCPP/SampleAppCpp.vcxproj

  <PropertyGroup>
    <ReactNativeWindowsDir Condition="'$(ReactNativeWindowsDir)' == ''">$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), 'node_modules\react-native-windows\package.json'))\node_modules\react-native-windows\</ReactNativeWindowsDir>
  </PropertyGroup>
-  <Import Project="$(ReactNativeWindowsDir)PropertySheets\React.Cpp.props" />


We shouldn't include React.cpp.props in projects external to the Microsoft.ReactNative boundary. We don't include it in the template project, and including it here actually led to it dropiing a ReactCommon folder on my C drive.

NickGerleman · 2020-04-21T22:35:41Z

vnext/Microsoft.ReactNative/ABIViewManager.cpp

 }

 folly::dynamic ABIViewManager::GetCommands() const {
-  folly::dynamic innerParent = Super::GetCommands();


Our superclass always returned nothing, so removing it doesn't do anything in practice, but I don't think this should have been here to begin with.

I.e. we don't dispatch to the superclass, so even if it did has commands to report, we could never actually run them.

shouldn't we fix the "dispatching to the superclass isn't working" problem then? Can you file an issue if you can't fix it in this PR?

I think it's fine to expect the Base View Manager won't handle anything in this case. Right now the only thing it does is assert(false) as a method to catch unhandled commands.

NickGerleman · 2020-04-21T22:37:37Z

vnext/ReactUWP/Views/SwitchViewManager.cpp

+    const folly::dynamic &commandArgs) {
+  if (commandId == "setValue") {
+    auto value = commandArgs[0].asBool();
+    viewToUpdate.as<winrt::ToggleSwitch>().IsEnabled(value);


This command is kind of weird. It's iOS only upstream, and in their impl doesn't touch native props either. From what I can tell it's to fix an iOS quirk, but implementing it doesn't seem to cause us any issues, and let's us fully share iOS JS code.

NickGerleman · 2020-04-22T00:17:16Z

We seem be be seeing different dumps for the internals of a switch between tests. This is possibly a timing issue.

Following up with @asklar but will otherwise keep Switch out of E2ETest to unblock fixing it.

asklar · 2020-04-22T02:15:16Z

...icrosoft-reactnative-sampleapps/windows/SampleLibraryCPP/CustomUserControlViewManagerCPP.cpp

  return commands.GetView();
 }

 void CustomUserControlViewManagerCpp::DispatchCommand(


since we are going to support both the int and string variants for the time being, should this be an overload rather than changing it to string so we can test both cases?

I touched on that a little in the PR description. Internal CPP code needs to support both, but the C++ view manager exposing IDs to implementers is a departure from the C# and ObjC view managers at least.

This has some motivation to move the direction of only strings, but the bigger concern I had was around API confusion. I.e. you look at an interface and see a command id version and a string version. It's super opaque to know which to implement when.

asklar · 2020-04-22T02:18:32Z

vnext/Microsoft.ReactNative/ABIViewManager.cpp

 }

 folly::dynamic ABIViewManager::GetCommands() const {
-  folly::dynamic innerParent = Super::GetCommands();


shouldn't we fix the "dispatching to the superclass isn't working" problem then? Can you file an issue if you can't fix it in this PR?

NickGerleman · 2020-04-22T02:23:29Z

vnext/Microsoft.ReactNative/ABIViewManager.cpp

-      innerParent.insert(key, value);
+    for (const auto &commandName : m_viewManagerWithCommands.Commands()) {
+      auto commandAsStr = to_string(commandName);
+      commandMap[commandAsStr] = commandAsStr;


On the Flow side, View Manager commands are untyped. Was unable to confirm whether other platforms have moved to fully using strings over the bridge yet though.

vmoroz · 2020-04-22T16:07:02Z

vnext/ReactUWP/Views/ViewManagerBase.cpp

 void ViewManagerBase::TransferProperties(XamlView /*oldView*/, XamlView /*newView*/) {}

+void ViewManagerBase::DispatchCommand(
+    XamlView /*viewToUpdate*/,


XamlView [](start = 4, length = 8)

Since it is a new method, could you pass the parameter correctly by const& ?

vmoroz · 2020-04-22T16:13:55Z

vnext/include/ReactUWP/Views/ViewManagerBase.h

  virtual void UpdateProperties(ShadowNodeBase *nodeToUpdate, const folly::dynamic &reactDiffMap);

+  virtual void DispatchCommand(XamlView viewToUpdate, const std::string &commandId, const folly::dynamic &commandArgs);
  virtual void DispatchCommand(XamlView viewToUpdate, int64_t commandId, const folly::dynamic &commandArgs);


XamlView viewToUpdate [](start = 31, length = 21)

XamlView is a smart pointer with additional info.
The best approach is to pass it by const& like we do for the std::string.

I agree. I'll try to bulk change everything in a followup PR instead of doing it just for this.

vmoroz

jonthysell

Delete the old id way internally if it's deprecated. Adding the commands for switch, we now have 3 total commands (1 in switch, 2 in scrollviewer) in the entire codebase.

jonthysell · 2020-04-22T17:28:31Z

vnext/include/ReactUWP/Views/ViewManagerBase.h

  virtual void UpdateProperties(ShadowNodeBase *nodeToUpdate, const folly::dynamic &reactDiffMap);

+  virtual void DispatchCommand(XamlView viewToUpdate, const std::string &commandId, const folly::dynamic &commandArgs);
  virtual void DispatchCommand(XamlView viewToUpdate, int64_t commandId, const folly::dynamic &commandArgs);


Why do we need to suport the older way? There are exactly 2 existing commands in the entire core, both on scrollviewer. Update scrollviewer to the new way and completely delete the old way.

I originally kept it because both still exist upstream (one marked for eventual removal), but I think you're right that we could just remove the old since we have control of our view managers and the string constant command trick.

In 0.61 Facebook added the ability to dispatch view manager commands using a string directly instead of the current indirection of getting a command ID from the view manager. This is leveraged in 0.62 in a couple places, including for Switches. UIManager overloads for string commands are added. This change impacts the C++ Microsoft.ReactNative ViewManager APIs C# and upstream view managers abstract away command IDs where they are currently left explicit in our C++ ViewManagers. Given our desire to implement built-in controls, we will need to support string commands here as well. This is made trickier with Facebook still publicly documenting grabbing command IDs from the VIewManager. Providing parallel explicit DispatchCommand functions to Microsoft.ReactNative ViewManagers would be too confusing to our users. Instead of adding parallel paths to the C++ ViewManager for commands without IDs, we remove the ID indirection entirely. Instead of internal ID mapping like we previously did with the C# ViewManagers, we now map constants to literal strings of themselves. This allows ViewManagers to accept the new form of literal string commands dispatched from JS without breaking the documented workflow of getting command IDs from the ViewManager. Eventually we should be able to remove integer only command IDs, as they are internally deprecated. Validated C# and C++ commands still work through the sample app. Validated switch behavior using the RNTester page. Fixes microsoft#4596

…ndows into stringviewmanagertemp

jonthysell · 2020-06-02T22:13:24Z

@NickGerleman Please update the viewmanager docs and examples in the docs repo to reflect these changes. Since they are in 62, you should do them before the website snaps to 62.

NickGerleman · 2020-06-02T22:17:50Z

@jonthysell thanks for the heads up! I missed our existing docs for view managers when I looked before, looking again, you're totally right that they exist and document the old API. Will change today.

NickGerleman added the Breaking Change This PR will break existing apps and should be part of the known breaking changes for the release label Apr 21, 2020

NickGerleman requested review from jonthysell and vmoroz April 21, 2020 22:32

NickGerleman requested a review from a team as a code owner April 21, 2020 22:32

NickGerleman commented Apr 21, 2020

View reviewed changes

asklar approved these changes Apr 22, 2020

View reviewed changes

NickGerleman commented Apr 22, 2020

View reviewed changes

vmoroz reviewed Apr 22, 2020

View reviewed changes

vmoroz approved these changes Apr 22, 2020

View reviewed changes

jonthysell approved these changes Apr 22, 2020

View reviewed changes

This was referenced Apr 22, 2020

Update react-native-webview with ViewManager API Changes Once 0.62 Preview Released #4679

Closed

Reenable Switches in E2ETest Once We Collapse Hidden Elements #4680

Closed

NickGerleman added 3 commits April 22, 2020 15:02

Merge branch 'master' of https://github.com/microsoft/react-native-wi…

7341d26

…ndows into stringviewmanagertemp

Change files

b105db4

NickGerleman force-pushed the string-commands branch from 194d554 to b105db4 Compare April 22, 2020 22:04

NickGerleman merged commit 509b9e5 into microsoft:master Apr 22, 2020

Implement String ViewManager Command IDs #4667

Implement String ViewManager Command IDs #4667

Uh oh!

Conversation

NickGerleman commented Apr 21, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickGerleman commented Apr 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmoroz left a comment

Choose a reason for hiding this comment

Uh oh!

jonthysell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonthysell commented Jun 2, 2020

Uh oh!

NickGerleman commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NickGerleman commented Apr 21, 2020 •

edited by ghost

Loading