RNW logs excessive "UnimplementedProperty" messages in debug builds

aschultz commented 2 years ago

Problem Description

RNW attempts to log "UnimplementedProperty" messages to indicate that a prop was passed to a native control but has not been handled by that control. However, the implementation of this messaging is incomplete and results in many extraneous messages.

The problem areas include:

All custom view managers (those constructed through ABIViewManager) will log the message for all custom properties (those not handled by the FrameworkElement hierarchy ABIViewManager extends).
All Text elements log the message for the isHighlighted prop that is used internally within the Text component implementation.

Steps To Reproduce

Create an RNW app with a custom native UI control that implements IViewManagerWithNativeProperties and render that element with a custom prop.
Build and run the RNW app through Visual Studio in Debug mode
Observe that the Output window logs an "UnimplementedProperty" message for the prop.
Create an RNW app with a Text element.
Build and run the RNW app through Visual Studio in Debug mode
Observe that the Output window logs an "UnimplementedProperty" message for the "isHighlighted" prop.

Expected Results

No "UnimplementedProperty" messages should be logged.

CLI version

7.0.3

Environment

System:
    OS: Windows 10 10.0.19044
    CPU: (12) x64 Intel(R) Xeon(R) CPU E5-1650 v3 @ 3.50GHz
    Memory: 12.01 GB / 31.92 GB
  Binaries:
    Node: 16.14.2 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.18 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 8.5.0 - C:\Program Files\nodejs\npm.CMD
    Watchman: Not Found
  SDKs:
    Android SDK: Not Found
    Windows SDK:
      AllowDevelopmentWithoutDevLicense: Enabled
      AllowAllTrustedApps: Enabled
      Versions: 10.0.10240.0, 10.0.10586.0, 10.0.14393.0, 10.0.15063.0, 10.0.16299.0, 10.0.17134.0, 10.0.17744.0, 10.0.17763.0, 10.0.18362.0, 10.0.19041.0
  IDEs:
    Android Studio: Not Found
    Visual Studio: 15.9.28307.1974 (Visual Studio Enterprise 2017), 16.11.32428.217 (Visual Studio Enterprise 2019)
  Languages:
    Java: Not Found
  npmPackages:
    @react-native-community/cli: 7.0.3 => 7.0.3
    react: 17.0.2 => 17.0.2
    react-native: 0.67.3 => 0.67.3
    react-native-windows: 0.67.6 => 0.67.6
  npmGlobalPackages:
    *react-native*: Not Found

Target Platform Version

No response

Target Device(s)

Desktop

Visual Studio Version

Visual Studio 2019

Build Configuration

Debug

Snack, code example, screenshot, or link to a repository

No response

asklar commented 2 years ago

isHighlighted is a JS property so the fact that it is showing up on the native side seems like a functional bug. Either we need the native code to implement it, or ignore it, or we should do something for JS to stop sending the update to us.

There is a followup item on being able to turn off the overall mechanism for UnimplementedProperty, or to make it smarter to work with ABI view managers.

aschultz commented 2 years ago

It looks like it's passed down to native on purpose. https://github.com/facebook/react-native/blob/7b5b114d578142d18bf4a7a5279b179a9ac8d958/Libraries/Text/Text.js

It shows up in a bunch of the RN native code. https://github.com/facebook/react-native/search?q=isHighlighted

NickGerleman commented 2 years ago

The code to log unknown properties to the debug console really ought to be removed in its entirety. It's intent was to find API parity gaps with upstream RN, but the mechanism is not detectable or actionable, multiple folks have reported it having a negative impact on developer experience, and it forced reshaping bits of the internal view manager APIs to accomadate it in some non-ideal ways. I don't think it has really ever been auctioned on, apart from maybe the initial point where it was merged in.

A while back I experimented with adding runtime tests, comparing the props the JS side knows about (via View Configs), vs what the native side claims to support. This catches properties without runtime usage, and fails tests when new ones are added, but even back then, it was known that code generation of the native component interface was the long term solution for this problem, since we can reason about any sort of missing properties at compile time. I think it would be best for any future work on API parity to use that shared solution. This is even more true now, with Fabric being out for Android and iOS.

rozele commented 1 year ago

isHighlighted is an iOS only prop that we don't need to implement for Windows. In fact, we recently noticed there was a bug in Fabric where it doesn't even work correctly on iOS 😅.

Agree with @NickGerleman that this should be disabled by default. Perhaps we should just hide it behind a #if that can be enabled when someone is doing parity investigations.

microsoft / react-native-windows