Enhancement: change to useScreens to enableScreens #99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
brentvatne
suggested changes
May 13, 2019
Contributor
Author
|
@brentvatne I've re-added the But if you prefer I can re-add that too. LMK |
Contributor
|
I think |
Contributor
Author
|
Ok, I've re-added the TS typing for useScreens - LMK if there is any other change needed. |
kmagiera
approved these changes
Oct 11, 2019
Member
kmagiera
left a comment
kmagiera
left a comment
There was a problem hiding this comment.
Wow, missed this one, I think it makes sense to have that change esp if its backward compatible. Thanks
|
Hey, can you bump a new release from this PR? Just being annoyed by eslint react hook complaining
|
Contributor
Author
|
oh lol I totally forgot about this PR XD We should also have in the release notes that this change is out there probably. |
1 task
kkafar
added a commit
that referenced
this pull request
Jul 16, 2025
This PR aims to hide any references to C++ symbols from the public API, *when consumed in C mode* while building Swift module. I add some indirection layers & hide all C++ headers from public API. For details see below (it's best to go commit by commit) 👇🏻 > [!note] > I do wonder why the compiler does not complain about rest of the files - we use C++ apis in "old" implementation excessively. My guess would be, that we're kinda lucky in similar fashion to mapbox - most likely RCT_NEW_ARCH_ENABLED is not defined for Swift compiler & all our usage of C++ symbols is behind that flag. I guess it's fine? It'll become a problem, when we get rid of Paper & remove the flag usage, however it is likely that we won't do that for the old impl (it'll be simply replaced by Fabric-only new impl). ## Testing The FabricExample builds fine in "regular setup". When building with static frameworks it fails due to gesture handler 👇🏻  If GH is removed, the build passes. Build for dynamic frameworks (tested only w/o GH) also passes locally. I can try bumping GH in follow up <details> <summary>Details of what changed (commits & descriptions)</summary> - **Migrate event emitting code of BottomTabsHost to dedicated class** This is a start of migration of library-wide refactor of our Objective-C++ code. We need to get rid of any references / imports of any C++ code / symbols in our header to make them consumable by our Swift implementation files. This particular commit migrates raw references to react event emitter into calls to wrapper object RNSBottomTabsHostEventEmitter. - **Add RNSReactBaseView** This class is meant to serve as a proxy for `RCTViewComponentView` for our component classes to inherit from. When needed this can be extended for old architecture. For views that do not need inheritance from `RCTViewComponentView` and `UIView<RCTComponentViewProtocol>` is enough separate proxy base class should be created. - **Use new RNSReactBaseView for RNSBottomTabsHostComponentView** This eliminates error with inheriting from "not existing" type. Type forward declaration is not enough unfortunately. - **Remove forward declaration for OnNativeFocusChangePayload** Unneeded. Can add it back, when we'll have multiple types, so that the main definition of RNSBottomTabsHostEventEmitter is not obfuscated. - **Define rnscreens::conversion namespace & nested functions only when in C++ mode** I believe this flag is not needed in implementation file, since .mm will be always compiled with C++. - **Use C-style struct defining syntax when not in C++ mode** I'm not confident in this change. Original code triggered an issue, that the symbol was missing. It was obviously due to struct definition syntax differences between C & C++. Therefore this commit splits C++ & C definitions. My main doubts are about memory layout of these structs & if these can be different & in this can lead to an runtime UB at any point. For now assuming that the type exposed to Swift here won't really matter. - **Make RNSStackScreenComponentView inherit from RNSReactBaseView** - **Make RNSScreenStackHostComponentView inherit from RNSReactBaseView** Also remove imports introducing any C++ symbols from .h file. - **Fix incorrect extension name for RNSBottomTabsScreenEventEmitter** - **Make all C++ headers "project" & set this on "common" subspec** Both {private,project}_header_files work here fine (in terms of project build), however from semantic standpoint "project" make more sense here. See nice summary of these two options: https://stackoverflow.com/a/8016333/8635522. Our C++ pieces are not meant to be consumed by any other target than us. We do not need to expose these symbols for anyone & we won't be consuming them in our Swift code. We might relax this later, when needed. Second thing is that this commit moves the setting from main spec to subspec. It made more sense for me, since the sources in the `common` & `cpp` directories belong to the subspec. At last - w/o this change build of Swift module will fail, complaining that it does not understand C++ symbols. - Fix issue with imports inside nullability audited region - **Make all our Swift symbols & method referenced in Objective-C public** Seems that this is requried for the symbols to be visible in Objective-C. This has obvious downsides - we leak unnecessary symbols to the public API, but this must suffice for now. We can look for a way to tighten things up later. See reference: https://silverhammermba.github.io/blog/2020/02/26/swift-and-obj-c </details>
kkafar
added a commit
that referenced
this pull request
Jul 16, 2025
## Description This PR aims to restore library functionality on iOS with the legacy architecture. It introduces necessary changes to make the project build & run, when no new components (gamma, bottom tabs) are referenced. ## Changes First, I've essentially copied the patches for RN we have already in place in `FabricExample`. This is a serious problem. In this state we can not publish the library as it won't build in client project and we can not ask users to patch the `react-native`. [The PR](cipolleschi/react-native@1d80586) responsible for fixing this already landed, however it haas not been published with 0.80 version, and has not been picked into `0.80.1` line. Even if it had, we aim to support at least 0.79 / 0.78. **Therefore we MUST find different way to work around this or resign from having Swift files in bottom tabs implementation.** Second, I've added necessary ifdefs to avoid C++ symbols being visible during Swift building. Fortunately I did not have to refactor whole v4 implementation in fashion similar to #99, because all C++ symbols during "Paper mode" build are hidden behind `RCT_NEW_ARCH_ENABLED` flags apparently, so we get the nice behaviour accidentally. ## Test code and steps to reproduce Run `Example` app. It should just build & run fine on iOS, e.g. on `Test42`. **It will crash** in runtime, when running tests using new API, however fixing it is outside of the scope of this PR. ## Checklist - [ ] Ensured that CI passes
kkafar
added a commit
that referenced
this pull request
Jul 16, 2025
This PR aims to hide any references to C++ symbols from the public API, *when consumed in C mode* while building Swift module. I add some indirection layers & hide all C++ headers from public API. For details see below (it's best to go commit by commit) 👇🏻 > [!note] > I do wonder why the compiler does not complain about rest of the files - we use C++ apis in "old" implementation excessively. My guess would be, that we're kinda lucky in similar fashion to mapbox - most likely RCT_NEW_ARCH_ENABLED is not defined for Swift compiler & all our usage of C++ symbols is behind that flag. I guess it's fine? It'll become a problem, when we get rid of Paper & remove the flag usage, however it is likely that we won't do that for the old impl (it'll be simply replaced by Fabric-only new impl). ## Testing The FabricExample builds fine in "regular setup". When building with static frameworks it fails due to gesture handler 👇🏻  If GH is removed, the build passes. Build for dynamic frameworks (tested only w/o GH) also passes locally. I can try bumping GH in follow up <details> <summary>Details of what changed (commits & descriptions)</summary> - **Migrate event emitting code of BottomTabsHost to dedicated class** This is a start of migration of library-wide refactor of our Objective-C++ code. We need to get rid of any references / imports of any C++ code / symbols in our header to make them consumable by our Swift implementation files. This particular commit migrates raw references to react event emitter into calls to wrapper object RNSBottomTabsHostEventEmitter. - **Add RNSReactBaseView** This class is meant to serve as a proxy for `RCTViewComponentView` for our component classes to inherit from. When needed this can be extended for old architecture. For views that do not need inheritance from `RCTViewComponentView` and `UIView<RCTComponentViewProtocol>` is enough separate proxy base class should be created. - **Use new RNSReactBaseView for RNSBottomTabsHostComponentView** This eliminates error with inheriting from "not existing" type. Type forward declaration is not enough unfortunately. - **Remove forward declaration for OnNativeFocusChangePayload** Unneeded. Can add it back, when we'll have multiple types, so that the main definition of RNSBottomTabsHostEventEmitter is not obfuscated. - **Define rnscreens::conversion namespace & nested functions only when in C++ mode** I believe this flag is not needed in implementation file, since .mm will be always compiled with C++. - **Use C-style struct defining syntax when not in C++ mode** I'm not confident in this change. Original code triggered an issue, that the symbol was missing. It was obviously due to struct definition syntax differences between C & C++. Therefore this commit splits C++ & C definitions. My main doubts are about memory layout of these structs & if these can be different & in this can lead to an runtime UB at any point. For now assuming that the type exposed to Swift here won't really matter. - **Make RNSStackScreenComponentView inherit from RNSReactBaseView** - **Make RNSScreenStackHostComponentView inherit from RNSReactBaseView** Also remove imports introducing any C++ symbols from .h file. - **Fix incorrect extension name for RNSBottomTabsScreenEventEmitter** - **Make all C++ headers "project" & set this on "common" subspec** Both {private,project}_header_files work here fine (in terms of project build), however from semantic standpoint "project" make more sense here. See nice summary of these two options: https://stackoverflow.com/a/8016333/8635522. Our C++ pieces are not meant to be consumed by any other target than us. We do not need to expose these symbols for anyone & we won't be consuming them in our Swift code. We might relax this later, when needed. Second thing is that this commit moves the setting from main spec to subspec. It made more sense for me, since the sources in the `common` & `cpp` directories belong to the subspec. At last - w/o this change build of Swift module will fail, complaining that it does not understand C++ symbols. - Fix issue with imports inside nullability audited region - **Make all our Swift symbols & method referenced in Objective-C public** Seems that this is requried for the symbols to be visible in Objective-C. This has obvious downsides - we leak unnecessary symbols to the public API, but this must suffice for now. We can look for a way to tighten things up later. See reference: https://silverhammermba.github.io/blog/2020/02/26/swift-and-obj-c </details>
kkafar
added a commit
that referenced
this pull request
Jul 16, 2025
## Description This PR aims to restore library functionality on iOS with the legacy architecture. It introduces necessary changes to make the project build & run, when no new components (gamma, bottom tabs) are referenced. ## Changes First, I've essentially copied the patches for RN we have already in place in `FabricExample`. This is a serious problem. In this state we can not publish the library as it won't build in client project and we can not ask users to patch the `react-native`. [The PR](cipolleschi/react-native@1d80586) responsible for fixing this already landed, however it haas not been published with 0.80 version, and has not been picked into `0.80.1` line. Even if it had, we aim to support at least 0.79 / 0.78. **Therefore we MUST find different way to work around this or resign from having Swift files in bottom tabs implementation.** Second, I've added necessary ifdefs to avoid C++ symbols being visible during Swift building. Fortunately I did not have to refactor whole v4 implementation in fashion similar to #99, because all C++ symbols during "Paper mode" build are hidden behind `RCT_NEW_ARCH_ENABLED` flags apparently, so we get the nice behaviour accidentally. ## Test code and steps to reproduce Run `Example` app. It should just build & run fine on iOS, e.g. on `Test42`. **It will crash** in runtime, when running tests using new API, however fixing it is outside of the scope of this PR. ## Checklist - [ ] Ensured that CI passes
maciekstosio
pushed a commit
that referenced
this pull request
Jul 17, 2025
This PR aims to hide any references to C++ symbols from the public API, *when consumed in C mode* while building Swift module. I add some indirection layers & hide all C++ headers from public API. For details see below (it's best to go commit by commit) 👇🏻 > [!note] > I do wonder why the compiler does not complain about rest of the files - we use C++ apis in "old" implementation excessively. My guess would be, that we're kinda lucky in similar fashion to mapbox - most likely RCT_NEW_ARCH_ENABLED is not defined for Swift compiler & all our usage of C++ symbols is behind that flag. I guess it's fine? It'll become a problem, when we get rid of Paper & remove the flag usage, however it is likely that we won't do that for the old impl (it'll be simply replaced by Fabric-only new impl). ## Testing The FabricExample builds fine in "regular setup". When building with static frameworks it fails due to gesture handler 👇🏻  If GH is removed, the build passes. Build for dynamic frameworks (tested only w/o GH) also passes locally. I can try bumping GH in follow up <details> <summary>Details of what changed (commits & descriptions)</summary> - **Migrate event emitting code of BottomTabsHost to dedicated class** This is a start of migration of library-wide refactor of our Objective-C++ code. We need to get rid of any references / imports of any C++ code / symbols in our header to make them consumable by our Swift implementation files. This particular commit migrates raw references to react event emitter into calls to wrapper object RNSBottomTabsHostEventEmitter. - **Add RNSReactBaseView** This class is meant to serve as a proxy for `RCTViewComponentView` for our component classes to inherit from. When needed this can be extended for old architecture. For views that do not need inheritance from `RCTViewComponentView` and `UIView<RCTComponentViewProtocol>` is enough separate proxy base class should be created. - **Use new RNSReactBaseView for RNSBottomTabsHostComponentView** This eliminates error with inheriting from "not existing" type. Type forward declaration is not enough unfortunately. - **Remove forward declaration for OnNativeFocusChangePayload** Unneeded. Can add it back, when we'll have multiple types, so that the main definition of RNSBottomTabsHostEventEmitter is not obfuscated. - **Define rnscreens::conversion namespace & nested functions only when in C++ mode** I believe this flag is not needed in implementation file, since .mm will be always compiled with C++. - **Use C-style struct defining syntax when not in C++ mode** I'm not confident in this change. Original code triggered an issue, that the symbol was missing. It was obviously due to struct definition syntax differences between C & C++. Therefore this commit splits C++ & C definitions. My main doubts are about memory layout of these structs & if these can be different & in this can lead to an runtime UB at any point. For now assuming that the type exposed to Swift here won't really matter. - **Make RNSStackScreenComponentView inherit from RNSReactBaseView** - **Make RNSScreenStackHostComponentView inherit from RNSReactBaseView** Also remove imports introducing any C++ symbols from .h file. - **Fix incorrect extension name for RNSBottomTabsScreenEventEmitter** - **Make all C++ headers "project" & set this on "common" subspec** Both {private,project}_header_files work here fine (in terms of project build), however from semantic standpoint "project" make more sense here. See nice summary of these two options: https://stackoverflow.com/a/8016333/8635522. Our C++ pieces are not meant to be consumed by any other target than us. We do not need to expose these symbols for anyone & we won't be consuming them in our Swift code. We might relax this later, when needed. Second thing is that this commit moves the setting from main spec to subspec. It made more sense for me, since the sources in the `common` & `cpp` directories belong to the subspec. At last - w/o this change build of Swift module will fail, complaining that it does not understand C++ symbols. - Fix issue with imports inside nullability audited region - **Make all our Swift symbols & method referenced in Objective-C public** Seems that this is requried for the symbols to be visible in Objective-C. This has obvious downsides - we leak unnecessary symbols to the public API, but this must suffice for now. We can look for a way to tighten things up later. See reference: https://silverhammermba.github.io/blog/2020/02/26/swift-and-obj-c </details>
maciekstosio
pushed a commit
that referenced
this pull request
Jul 17, 2025
## Description This PR aims to restore library functionality on iOS with the legacy architecture. It introduces necessary changes to make the project build & run, when no new components (gamma, bottom tabs) are referenced. ## Changes First, I've essentially copied the patches for RN we have already in place in `FabricExample`. This is a serious problem. In this state we can not publish the library as it won't build in client project and we can not ask users to patch the `react-native`. [The PR](cipolleschi/react-native@1d80586) responsible for fixing this already landed, however it haas not been published with 0.80 version, and has not been picked into `0.80.1` line. Even if it had, we aim to support at least 0.79 / 0.78. **Therefore we MUST find different way to work around this or resign from having Swift files in bottom tabs implementation.** Second, I've added necessary ifdefs to avoid C++ symbols being visible during Swift building. Fortunately I did not have to refactor whole v4 implementation in fashion similar to #99, because all C++ symbols during "Paper mode" build are hidden behind `RCT_NEW_ARCH_ENABLED` flags apparently, so we get the nice behaviour accidentally. ## Test code and steps to reproduce Run `Example` app. It should just build & run fine on iOS, e.g. on `Test42`. **It will crash** in runtime, when running tests using new API, however fixing it is outside of the scope of this PR. ## Checklist - [ ] Ensured that CI passes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This wants to be a small PR to improve the usability of the library, based on this conversation: https://twitter.com/grifotv/status/1127847192067215360.
Since the release of RNS there has been a new major player in the React game: hooks. And sadly
useScreensrecalls too closely Hooks, and this can lead to misunderstanding.Changing it to
enableScreenswill make the difference clear, but at the same time it will be a BREAKING CHANGE for everyone using the lib.So if we prefer to keep it around as useScreens I'm ok with it too, and we can close this.
Also, I did some tweaks to the README + fix some typos.