microsoft / react-native-windows-samples

A repository showcasing React Native samples and templates for Windows, macOS, and Surface Duo.
https://microsoft.github.io/react-native-windows/
MIT License
477 stars 200 forks source link

FancyMath walkthrough in docs has drifted from what's checked into the samples #797

Open chrisglein opened 1 year ago

chrisglein commented 1 year ago

Page url

https://microsoft.github.io/react-native-windows/docs/native-modules#5-using-your-native-module-in-js

Problem Description

If you try to copy and paste those code snippets you'll end up with part using Pi and part using PI.

image

image

image

The one in NativeFancyMath.ts is wrong, right?

asklar commented 1 year ago

I believe this is intentional to showcase that you can override the projected name:

The [ReactConstant] attribute is how you can define constants. Here FancyMath has defined two constants: E and Pi. By default, the name exposed to JS will be the same name as the field (E for E), but you can override the name like this: [ReactConstant("Pi")].

chrisglein commented 1 year ago

I get the remapping with ReactConstant("Pi"). But shouldn't the native spec use the "Pi" version and not "PI"?

If you look at the implementation in the repo it says "Pi" in the native spec.

But also it has syntax I don't understand with +'s and |'s in there. So... 🤷‍♂️

   // Exported methods.
   +getConstants: () => {|
     E: number,
     Pi: number,
   |};
   +add: (a: number, b: number, callback: (value: number) => void) => void;
chrisglein commented 1 year ago

Also took me awhile to find out what else needed to change:

add(a: number, b: number): Promise<number>;

To this:

add(a: number, b: number, callback: (value: number) => void): void;
chrisglein commented 1 year ago

Updating title beyond Pi, because there seem to be other descrepencies:

from the docs

#include "pch.h"
#include "ReactPackageProvider.h"
#include "ReactPackageProvider.g.cpp"

#include <ModuleRegistration.h>

// NOTE: You must include the headers of your native modules here in
// order for the AddAttributedModules call below to find them.
#include "FancyMath.h"

namespace winrt::NativeModuleSample::implementation
{
    void ReactPackageProvider::CreatePackage(IReactPackageBuilder const& packageBuilder) noexcept
    {
        AddAttributedModules(packageBuilder, true);
    }
}

from the sample

// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

#include "pch.h"
#include "ReactPackageProvider.h"
#include "FancyMath.h"
#include "DataMarshallingExamples.h"
#include "AsyncMethodExamples.h"
#if __has_include("ReactPackageProvider.g.cpp")
#include "ReactPackageProvider.g.cpp"
#endif

using namespace winrt::Microsoft::ReactNative;

namespace winrt::NativeModuleSample::implementation
{

void ReactPackageProvider::CreatePackage(IReactPackageBuilder const &packageBuilder) noexcept
{
    AddAttributedModules(packageBuilder, true);
}

} // namespace winrt::NativeModuleSample::implementation

Why the difference in include of ReactPackageProvider.g.cpp and lack of .h? Why the difference in <ModuleRegistration.h>? Are these meaningful differences?

Additionally the ReactPackageProvider is defined differently between this and what the app template creates. Which is correct?

from the docs/sample

#pragma once

#include "ReactPackageProvider.g.h"

using namespace winrt::Microsoft::ReactNative;

namespace winrt::NativeModuleSample::implementation
{
    struct ReactPackageProvider : ReactPackageProviderT<ReactPackageProvider>
    {
        ReactPackageProvider() = default;

        void CreatePackage(IReactPackageBuilder const& packageBuilder) noexcept;
    };
}

namespace winrt::NativeModuleSample::factory_implementation
{
    struct ReactPackageProvider : ReactPackageProviderT<ReactPackageProvider, implementation::ReactPackageProvider> {};
}

from the template

#pragma once

#include "winrt/Microsoft.ReactNative.h"

namespace winrt::rngallery::implementation
{
    struct ReactPackageProvider : winrt::implements<ReactPackageProvider, winrt::Microsoft::ReactNative::IReactPackageProvider>
    {
    public: // IReactPackageProvider
        void CreatePackage(winrt::Microsoft::ReactNative::IReactPackageBuilder const &packageBuilder) noexcept;
    };
} // namespace winrt::rngallery::implementation
chrisglein commented 1 year ago

Use of constants is wrong for Turbo Modules (which will be the behavior in release mode):

      <View>
         <Text>FancyMath says PI = {FancyMath.Pi}</Text>
         <Text>FancyMath says E = {FancyMath.E}</Text>
         <Button onPress={this._onPressHandler} title="Click me!"/>
      </View>);

Should use FancyMath.getConstants().Pi, for example.