[RNMobile] Relocate useNetworkConnectivity hook and withNetworkConnectivity HOC #59982
Conversation
This file contains 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
derekblank
added
Mobile App - i.e. Android or iOS
derekblank
added
the
[Type] Task
|
@dcalhoun |
|
Size Change: +509 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
I've relocated It appears to be working fine (including tests). However, since we intend to also relocate usePreferredColorScheme in a separate PR, I'm curious if there are alternate ways to organize and export these hooks within I.e., is there something more concise than the following: |
dcalhoun
reviewed
Mar 20, 2024
There was a problem hiding this comment.
@derekblank it appears the tests fail. I see the reported error when running the Demo editor app on iOS locally.
However, since we intend to also relocate usePreferredColorScheme in a separate PR, I'm curious if there are alternate ways to organize and export these hooks within
react-native-editorpackage index.I.e., is there something more concise than the following:
I'm not confident I understand your thought regarding alternatives or more concise approaches for exports, but one pattern is the following.
export { default as uniqueName } from './path/to/module';Additionally, if the intention is to expand the logic managed by @wordpress/react-native-editor, we might consider organizing the files to mirror other packages — e.g., adding a hooks directory to house Hook files rather than having all files placed in the top-level directory.
| usePrevious, | ||
| } from '@wordpress/compose'; | ||
| import { usePreferredColorSchemeStyle, usePrevious } from '@wordpress/compose'; | ||
| import { useNetworkConnectivity } from '@wordpress/react-native-editor'; |
There was a problem hiding this comment.
This appears to be the first ever import of @wordpress/react-native-editor. I.e., no other code depends upon this package.
This causes me to wonder if placing these files in @wordpress/react-native-bridge makes more sense, given the code in question directly interfaces with bridge methods. Obviously, this conflicts with @fluiddot's comment on placing these in @wordpress/react-native-editor, so I am eager to hear his thoughts as well. I will note this would add a @wordpress/element as a dependency for @wordpress/react-native-bridge — unsure if that is something to avoid.
The @wordpress/react-native-editor is described as "a demo application" even though it is used in gutenberg-mobile to generate the module utilized by the host apps. So, there is likely room to better define its purpose, updating documentation and structure.
WDYT?
There was a problem hiding this comment.
This causes me to wonder if placing these files in
@wordpress/react-native-bridgemakes more sense, given the code in question directly interfaces with bridge methods. Obviously, this conflicts with @fluiddot's comment on placing these in@wordpress/react-native-editor, so I am eager to hear his thoughts as well. I will note this would add a@wordpress/elementas a dependency for@wordpress/react-native-bridge— unsure if that is something to avoid.The
@wordpress/react-native-editoris described as "a demo application" even though it is used ingutenberg-mobileto generate the module utilized by the host apps. So, there is likely room to better define its purpose, updating documentation and structure.
From my POV, I see react-native-bridge as a package that only holds the bridge functionality. Whilst, the react-native-editor package exposes root-level editor functions (e.g. registerGutenberg). It's true that currently react-native-editor only mentions the demo editor, but I'd propose that it serves a wider purpose. As you pointed out @dcalhoun, I agree there's room to update and improve the documentation to better reflect the purpose of the package.
There was a problem hiding this comment.
@fluiddot your perspective makes sense. I can support that. 👍🏻
Regardless of where we place the files, we should attempt to ensure were are not introducing new cyclic dependencies. Ideally, we address some existing ones with this change.
There was a problem hiding this comment.
Regardless of where we place the files, we should attempt to ensure were are not introducing new cyclic dependencies. Ideally, we address some existing ones with this change.
Good point. We have a bunch of cyclic dependencies, so I'd totally advocate avoiding adding more 😅 .
There was a problem hiding this comment.
I agree with the approach of relocating the hooks to react-native-editor package. For next steps, I'll work on a hooks organization strategy in this package, and update the documentation to expand the purpose of the package.
We should attempt to ensure were are not introducing new cyclic dependencies.
@dcalhoun Do you already note where there are cyclic dependencies created by locating these hooks within react-native-editor, or is this simply a cautionary check to be aware of them?
There was a problem hiding this comment.
Caution. All of the cyclic dependencies are listed when the Metro server starts. We should review them and ensure we do not add more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
Relocates
useNetworkConnectivityhook andwithNetworkConnectivityHOC from the@wordpress/composepackage to@wordpress/editor.Why?
Removes one of the dependencies of
react-native-bridgefrom the@wordpress/composepackage. See further discussion on #59939.How?
Relocates the files for
useNetworkConnectivityandwithNetworkConnectivityto@wordpress/editorand updates references as needed.Testing Instructions