iOS SceneDelegate migration proposal#967
iOS SceneDelegate migration proposal#967artus9033 wants to merge 7 commits intoreact-native-community:mainfrom
Conversation
9c54321 to
2d3189d
Compare
cipolleschi
left a comment
There was a problem hiding this comment.
Good job here!
I left a bunch of comments that should be applied/adressed in the RFC.
Make sure not to have any RN acronym. In official documentation we always use the full React Native product name.
| #### Migrating Dimensions: internal state adjustment & naming convention | ||
|
|
||
| Dimensions currently are exported as a constant. Such design does not fit the concept of resizable windows. To accommodate this, `RCTDeviceInfo` (iOS native module) should update its internal state variable upon changes to the frame so as to make `getConstants` return a value that is up-to-date at the time of invocation. | ||
|
|
||
| `Dimensions` (JS API) contains a `getConstants` method that wraps the native `getConstants` method and caches the result internally. The caching would now be obsolete (since the value may change in time), therefore the underlying native method should always be invoked. Moreover, the naming of the JS `getConstants` method in face of resizable windows may be misleading, since from now on the dimensions are not constant. Therefore, I propose a gradual adoption of a new method of same functionality, `getInfo()`, along with the deprecation of `getConstants()`, on the JS API side. A rough draft of the JS API changes would be: | ||
|
|
||
|  |
There was a problem hiding this comment.
I don't think this is actually required for the SceneDelegate migration. probably we can move it to a different RFC/effort
There was a problem hiding this comment.
Agreed, I will move it from here, then
There was a problem hiding this comment.
I opened a separate RFC to track this idea: #969 - CC @cipolleschi
7301b34 to
e343215
Compare
|
Thank you for the comments Riccardo! Yes, sorry for not using the full name of React Native - I will keep that in mind. I think the RFC is now ready for a re-review, I applied your suggestions and also reworded few sections. |
| Adoption of `UIScene` lifecycle requires the following actions: | ||
|
|
||
| * In application base code | ||
| * migration from `AppDelegate` as the primary point of lifecycle-related logic to `SceneDelegate`; for backwards compatibility, React Native's public API integration points will still be compatible with `AppDelegate` approach for users that may not want to migrate immediately |
There was a problem hiding this comment.
Think it would be ok to not have that backwards compatibility. The Delegate is on a clock anyway that we do not control, so everyone NEEDS to migrate, and keeping the old approach around is just delaying an inevitable that will be significantly more painful then and we would need to live the double life in the mean time supporting both approaches.
Might be best to just say, from this version of RN, in order to be compliant with Apple's requirements, we are moving off, older versions not affected. Just being clear in the comms with the framework end users
There was a problem hiding this comment.
I agree on the point that this has to be removed at some stage, yet the objective I had in mind was to bring an additive, non-breaking change. We should definitely deprecate those methods, but a sudden removal would make an unpleasant surprise for the users. By deprecating, we make it a gentle migration and we'll get to removal anyway.
There was a problem hiding this comment.
I agree with Parsa. It is better to do this properly once rather than carrying things forward just for backward compatibility. Many things in the core break over time without being visible to us, and this is not an exception.
Another issue is the terminology itself. In the React Native ecosystem, the term “users” is often used to refer to consumers of a package, which easily causes confusion. In our case, those “users” are developers, not end users. Since the audience is developers, they are expected to adapt to changes and learn new patterns. We are not building a product for non-technical end customers.
There was a problem hiding this comment.
Thanks for sharing that, then I think the matter is up to be discussed. WDYT @cipolleschi ?
Regarding the "users", definitely the user persona I was referring to in this context was the developer - yet, I'll keep in mind that the default meaning here is the end user, thank you for noting that!
|
Hey, this is great work. I'm exploring CarPlay support in a React Native/Expo app where CarPlay would be implemented natively in Swift using CPTemplate APIs, while the main app stays in React Native. A few questions about the proposed API:
|
Thank you, it's a pleasure to hear that! Regarding your questions:
I think it should not collide in any way, since scene management happens in the driver code of your application: - (void)scene:(UIScene *)scene
willConnectToSession:(UISceneSession *)session
options:(UISceneConnectionOptions *)connectionOptions
{
// ...
RCTReactNativeFactory *factory = [[RCTReactNativeFactory alloc] initWithDelegate:delegate];
// ...
[factory startReactNativeWithModuleName:@"RNTesterApp"
inWindow:self.window
initialProperties:[self prepareInitialProps]
connectionOptions:connectionOptions];
}That said, React Native operates only in the window in the scene you start it with.
I think this is a question more related to brownfield, this PR does not change anything w.r.t. the previous state, but anyway the factory should be initialized once from what I remember - as in this piece of code in react-native-brownfield.
Just to confirm, by that you mean creating some scene roles that never have anything to do with React Native? I haven't done any experiments in such a scenario, but don't foresee anything that would break - the code path for initializing the RN scene would just never be hit, I guess. |
A proposal for adding
SceneDelegatesupport to iOS.View the rendered RFC