search

microsoft / react-native-windows

A framework for building native Windows apps with React.
https://microsoft.github.io/react-native-windows/
Other
16.3k stars 1.14k forks source link

Document RNW threading requirements #6748

Open asklar opened 3 years ago

asklar commented 3 years ago

The goal here is to document the threading model used by the RNW and the native modules.

@vmoroz :

I plan to do the doc updates next week. This week I want to make sure that the core functionality works.

In reply to: 543014346 [](ancestors = 543014346)

_Originally posted by @vmoroz in https://github.com/microsoft/react-native-windows/pull/6745#discussion_r543019314_

ghost commented 3 years ago

Many of our core react-native-windows contributors are taking some much needed vacation throughout December 2020. Thank you for being patient while we relax, recharge, and return to a normal responsiveness in January 2021. In the meantime feel free to continue to pose questions, open issues, and make feature requests - you just may not get a response right away.

vmoroz commented 3 years ago

The docs are written in PR #7393, The 0.64 port is the PR #7503

asklar commented 3 years ago

@vmoroz main thing I wanted to capture in this issue, if I remember correctly, were the threading requirements; do we have those in the docs?

vmoroz commented 3 years ago

@asklar, I misunderstood the issue: I thought it is referring to documenting IReactSettingsSnapshot interface that was also required to be documented at that time. The title of this issue is misleading. There are no ReactNative.InstanceData.* properties that we want to document. Many of them are used internally and ideally should not be used directly by module writers. Could you please update the title and the description of the issue to specify exactly what is the expectation here. I re-read the context of the PR where we have the link and everything what I said there is that each React instance has its own settings - I would assume that it is quite obvious information that can be understood from our API structure and existing docs.

asklar commented 3 years ago

refreshing my own memory :) I think the InstanceData properties that the issue refers to are these properties: https://github.com/microsoft/react-native-windows/blob/91fcecf2947f4db638f795ed46640f5adbc9520d/vnext/Microsoft.ReactNative.Cxx/JSI/JsiApiContext.cpp#L37

If these properties are an internal implementation detail and we don't expect modules/apps to ever interact with them then that's fine. Do we have to do anything to reserve these so they're not overwritten/messed with by the app or modules?

The comment in the PR mentioned this part which I don't know whether it is obvious from the existing docs, what do you think? (emphasis mine):

Different instances can have property with the same name, but different values. But they must use different threads

vmoroz commented 3 years ago

The comment in the PR mentioned this part which I don't know whether it is obvious from the existing docs, what do you think? (emphasis mine):

Different instances can have property with the same name, but different values. But they must use different threads

This is the current implementation detail where we use a dedicated thread to run JavaScript code for each instance. It should be OK for cases when the whole App uses a few React instances. But when we use ReactNative as plugins for Office, it may cause inefficient resource usage. Ideally we must switch off the dedicated thread model.

For the comment above But they must use different threads - I was just stating how the system works today. It does not require any special actions from developers. It was mostly FYI.

What if we transform this issue to a requirement to document the threading model used by RNW? There we can say how it works today and then mention how we want to change it in future. E.g. we should ask not to use the TLS like we do in that code. :)

asklar commented 3 years ago

Sounds good to me, updating title