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

Implement the SampleTurboModule module #13531

Closed
jonthysell opened this issue Aug 2, 2024 · 1 comment · Fixed by #13541
Closed

Implement the SampleTurboModule module #13531

jonthysell opened this issue Aug 2, 2024 · 1 comment · Fixed by #13541
Assignees
@jonthysell
Labels
Area: Turbo Modules enhancement New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric
Milestone
Next

Comments

@jonthysell
Copy link
Contributor

Summary

We currently don't implement the SampleTurboModule module properly. Instead we have a hook in TurboModuleManager that, when SampleTurboModule is asked for, we just return a duplicated of the older (but until now very similar) SampleTurboCxxModule.

Motivation

We need to implement the module for real because the example pages in RNTester are starting to use the new functionality of the module that the older SampleTurboCxxModule does not implement. This starts as of the 7/1 integrate (see #13508).

Basic Example

No response

Open Questions

No response
@microsoft-github-policy-service microsoft-github-policy-service bot added New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Aug 2, 2024
@jonthysell
Copy link
Contributor Author

While we can start making this right away, we won't be able to implement the new EventEmitter APIs until #13532 is completed.

jonthysell added a commit to jonthysell/react-native-windows that referenced this issue Aug 2, 2024
@jonthysell
Implement the SampleTurboModule module
59010d9 
This PR provides a proper implementation of the `SampleTurboModule`
module, and removes the proxy code in `TurboModuleManager` which instead
substituted the old `SampleTurboCxxModule` module.

Closes microsoft#13531
@jonthysell jonthysell mentioned this issue Aug 2, 2024
Closed
jonthysell added a commit to jonthysell/react-native-windows that referenced this issue Aug 2, 2024
@jonthysell
Implement the SampleTurboModule module
a1b543b 
This PR provides a proper implementation of the `SampleTurboModule`
module, and removes the proxy code in `TurboModuleManager` which instead
substituted the old `SampleTurboCxxModule` module.

Closes microsoft#13531
@jonthysell jonthysell self-assigned this Aug 5, 2024
@jonthysell jonthysell removed the Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) label Aug 5, 2024
@jonthysell jonthysell added this to the Next milestone Aug 5, 2024
@jonthysell jonthysell added Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) and removed Needs: Triage 🔍 New issue that needs to be reviewed by the issue management team (label applied by bot) labels Aug 5, 2024
@jonthysell jonthysell mentioned this issue Aug 6, 2024
Merged
Yajur-Grover pushed a commit to Yajur-Grover/react-native-windows that referenced this issue Oct 9, 2024
@jonthysell @Yajur-Grover
Implement the SampleTurboModule module (microsoft#13541)
a63011c 
This PR provides a proper implementation of the `SampleTurboModule` module and removes the proxy code in `TurboModuleManager` which instead substituted the old `SampleTurboCxxModule` module.

- New feature (non-breaking change which adds functionality)

The APIs of `SampleTurboModule` are starting to deviate from the older `SampleTurboCxxModule`, specifically the addition of new `EventEmitter` members. So it's time we had a "real" implementation of `SampleTurboModule`.

Closes microsoft#13531

See above.

N/A

Verified tests still pass and the new module is being called.

Should this change be included in the release notes: _yes_

Implement the SampleTurboModule module
Yajur-Grover added a commit that referenced this issue Oct 11, 2024
@Yajur-Grover @sharath2727
@acoates-ms @chiaramooney @jonthysell
[0.75] Cherry-pick Fabric changes (#13948)
91192a3 
* Introduce ReactNativeAppBuilder (#13368)

* Introduce ReactNativeAppBuilder.idl

* yarn format fix

* Change files

* Address feedback

* Add a fabric check for ReactNativeAppBuilder

* yarn format

* Update reactnativeappbuilder (#13467)

* Build reactnativeappbuilder

* Setup reactnativewin32 app

* Fix dispatcherqueue issue

* Change files

* Cleanup

* Address feedback

* Address feedback2

* Fix pipeline error

* Fix pipeline errors

* Address feedback

* [Fabric] Fix crash when running inspect (#13592)

* [Fabric] Fix crash when running inspect

* Change files

* fix

* [Fabric] LogBox should destroy its window on instance shutdown (#13675)

* [Fabric] LogBox should destroy its window on instance shutdown

* Change files

* format

* [Fabric] Add FocusNavigationDirection and allow overriding of default command handling (#13857)

* [Fabric] Add FocusNavigationDirection and allow overriding of default command handling

* Change files

* revert packages.lock

* [Fabric] LayoutDirection and FontSizeMultiplier support (#13866)

* [Fabric] LayoutDirection and FontSizeMultiplier support

* Change files

* Missed a few places not setting LayoutDirection

* fix crash on logbox

* Fix textinput caret

* fix

* RichEdit has to be told its multiline before we can set a multiline string

* Update SampleAppFabric to use ReactNativeAppBuilder (#13382)

* Update SampleAppFabric to use ReactNativeAppBuilder

* Yarn format fix

* Address feedback to keep BUNDLE and DEBUG

* Yarn format

* Change files

* Setup the DebugBundlePath when loading from metro

* fix typo

* [Fabric] Enable ScrollView touch scrolling (#13664)

* [Fabric] Enable ScrolView touch scrolling

* Change files

* format

* typo

* [Fabric] Add Support for accessibilityActions and onAccessibilityAction (#13674)

* Add Implementation for accessibilityActions

* Add activate action

* Format

* Change files

* Use a legacy native module binding that always returns null in bridgeless mode (#13905)

* Use a legacy native module binding that always returns null in bridgeless mode

* Change files

* format

* SampleTurboModule only works as a turbomodule, so do not install it when using web debugger (#13911)

* SampleTurboModule only works as a turbomodule, so do not install it when using web debugger

* Change files

* fix

* Implement the SampleTurboModule module (#13541)

This PR provides a proper implementation of the `SampleTurboModule` module and removes the proxy code in `TurboModuleManager` which instead substituted the old `SampleTurboCxxModule` module.

- New feature (non-breaking change which adds functionality)

The APIs of `SampleTurboModule` are starting to deviate from the older `SampleTurboCxxModule`, specifically the addition of new `EventEmitter` members. So it's time we had a "real" implementation of `SampleTurboModule`.

Closes #13531

See above.

N/A

Verified tests still pass and the new module is being called.

Should this change be included in the release notes: _yes_

Implement the SampleTurboModule module

* fix previous merge

* fix bad merge

* fix more typos

* update snapshots

---------

Co-authored-by: Sharath Manchala <[email protected]>
Co-authored-by: Andrew Coates <[email protected]>
Co-authored-by: Chiara Mooney <[email protected]>
Co-authored-by: Jon Thysell <[email protected]>
acoates-ms pushed a commit to acoates-ms/react-native-windows that referenced this issue Oct 11, 2024
@jonthysell @acoates-ms
Implement the SampleTurboModule module (microsoft#13541)
48244e9 
This PR provides a proper implementation of the `SampleTurboModule` module and removes the proxy code in `TurboModuleManager` which instead substituted the old `SampleTurboCxxModule` module.

- New feature (non-breaking change which adds functionality)

The APIs of `SampleTurboModule` are starting to deviate from the older `SampleTurboCxxModule`, specifically the addition of new `EventEmitter` members. So it's time we had a "real" implementation of `SampleTurboModule`.

Closes microsoft#13531

See above.

N/A

Verified tests still pass and the new module is being called.

Should this change be included in the release notes: _yes_

Implement the SampleTurboModule module
acoates-ms added a commit that referenced this issue Oct 15, 2024
@acoates-ms @sharath2727
@jonthysell @chiaramooney @rnbot
[0.74] Backport various fabric fixes (#13960)
cc7dea7 
* Introduce ReactNativeAppBuilder (#13368)

* Introduce ReactNativeAppBuilder.idl

* yarn format fix

* Change files

* Address feedback

* Add a fabric check for ReactNativeAppBuilder

* yarn format

* Update SampleAppFabric to use ReactNativeAppBuilder (#13382)

* Update SampleAppFabric to use ReactNativeAppBuilder

* Yarn format fix

* Address feedback to keep BUNDLE and DEBUG

* Yarn format

* Change files

* Setup the DebugBundlePath when loading from metro

* Update reactnativeappbuilder (#13467)

* Build reactnativeappbuilder

* Setup reactnativewin32 app

* Fix dispatcherqueue issue

* Change files

* Cleanup

* Address feedback

* Address feedback2

* Fix pipeline error

* Fix pipeline errors

* Address feedback

* Implement the SampleTurboModule module (#13541)

This PR provides a proper implementation of the `SampleTurboModule` module and removes the proxy code in `TurboModuleManager` which instead substituted the old `SampleTurboCxxModule` module.

- New feature (non-breaking change which adds functionality)

The APIs of `SampleTurboModule` are starting to deviate from the older `SampleTurboCxxModule`, specifically the addition of new `EventEmitter` members. So it's time we had a "real" implementation of `SampleTurboModule`.

Closes #13531

See above.

N/A

Verified tests still pass and the new module is being called.

Should this change be included in the release notes: _yes_

Implement the SampleTurboModule module

* [Fabric] Fix crash when running inspect (#13592)

* [Fabric] Fix crash when running inspect

* Change files

* fix

* [Fabric] Enable ScrollView touch scrolling (#13664)

* [Fabric] Enable ScrolView touch scrolling

* Change files

* format

* typo

* [Fabric] Add Support for accessibilityActions and onAccessibilityAction (#13674)

* Add Implementation for accessibilityActions

* Add activate action

* Format

* Change files

* [Fabric] LogBox should destroy its window on instance shutdown (#13675)

* [Fabric] LogBox should destroy its window on instance shutdown

* Change files

* format

* [Fabric] Add FocusNavigationDirection and allow overriding of default command handling (#13857)

* [Fabric] Add FocusNavigationDirection and allow overriding of default command handling

* Change files

* revert packages.lock

* [Fabric] LayoutDirection and FontSizeMultiplier support (#13866)

* [Fabric] LayoutDirection and FontSizeMultiplier support

* Change files

* Missed a few places not setting LayoutDirection

* fix crash on logbox

* Fix textinput caret

* fix

* RichEdit has to be told its multiline before we can set a multiline string

* Use a legacy native module binding that always returns null in bridgeless mode (#13905)

* Use a legacy native module binding that always returns null in bridgeless mode

* Change files

* format

* SampleTurboModule only works as a turbomodule, so do not install it when using web debugger (#13911)

* SampleTurboModule only works as a turbomodule, so do not install it when using web debugger

* Change files

* fix

* merge conflicts

* format

* fix

* lint fix

* fix

* fix

* update snapshots

---------

Co-authored-by: Sharath Manchala <[email protected]>
Co-authored-by: Jon Thysell <[email protected]>
Co-authored-by: Chiara Mooney <[email protected]>
Co-authored-by: React-Native-Windows Bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jonthysell jonthysell

Labels
Area: Turbo Modules enhancement New Architecture Broad category for issues that apply to the RN "new" architecture of Turbo Modules + Fabric
Projects
RNW New Architecture
Status: Done
Milestone
Next
1 participant
@jonthysell