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.

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

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

Open
chrisglein opened this issue Mar 14, 2023 · 5 comments
Labels
bug Something isn't working documentation Improvements or additions to documentation
Milestone

Comments

@chrisglein
Copy link
Member

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?

@chrisglein chrisglein added bug Something isn't working documentation Improvements or additions to documentation labels Mar 14, 2023
@asklar
Copy link
Member

asklar commented Mar 14, 2023

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
Copy link
Member Author

chrisglein commented Mar 14, 2023

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
Copy link
Member Author

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 chrisglein changed the title FancyMath example doc mixes use of Pi and PI FancyMath walkthrough in docs has drifted from what's checked into the samples Mar 15, 2023
@chrisglein
Copy link
Member Author

chrisglein commented Mar 15, 2023

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
Copy link
Member Author

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.

acoates-ms added a commit that referenced this issue Mar 16, 2023
## Description
TurboModule docs do not mention or show the need to use getConstants()
to access constants. And a couple of other minor fixes

Fixes at least some of the issues from #797 
###### Microsoft Reviewers: [Open in
CodeFlow](https://portal.fabricbot.ms/api/codeflow?pullrequest=https://github.com/microsoft/react-native-windows-samples/pull/799)
###### Microsoft Reviewers: [Open in
CodeFlow](https://portal.fabricbot.ms/api/codeflow?pullrequest=https://github.com/microsoft/react-native-windows-samples/pull/799)
@chrisglein chrisglein modified the milestones: 0.72, 0.73 Oct 18, 2023
@TatianaKapos TatianaKapos removed their assignment Nov 7, 2023
@TatianaKapos TatianaKapos modified the milestones: 0.73, Backlog Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation
Projects
None yet
Development

No branches or pull requests

3 participants