[RNMobile] Upgrade react-native-modal and other warning fixes #32772
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
To fix "Animated.event now requires a second argument for options" caused by using a fairly old version of the lib.
|
Size Change: +93 B (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
2 tasks
This reverts commit 1e05044.
Which is also the recommended way to add subscriptions. See https://reactjs.org/blog/2018/03/27/update-on-async-rendering.html#adding-event-listeners-or-subscriptions
This reverts commit d6cf2f8.
Label-less Toolbar is deprecated https://github.com/WordPress/gutenberg/blob/7fc4197a6494c040fc3d4e491f22602e058fb126/packages/components/src/toolbar/index.js#L30-L35 Tried to add labels with d6cf2f8 but introduced issues: the keyboard dismiss button was out of place.
hypest
changed the title
dcalhoun
approved these changes
Jun 17, 2021
There was a problem hiding this comment.
LGTM. I tested the changes on an iPhone 12 mini simulator and Samsung Galaxy S20.
I did see a few additional logs. The first is related to this PR, the others are not. I'll leave to your digression if we address them in this PR.
Related
- Navigate to Cover block > Settings > Focal point picker:
Animated.event now requires a second argument for options
Unrelated
Require cycles are allowed but can result in uninitialized values. Consider refactoring to remove the need for a cycle.New content generated by 'save' functionfor Cover and Group blocks. (Mobile App: Fix warning reported in the terminal when running the test app #29655)Removing lineHeight style as it's not supported by native AztecView(I know this was intentionally added, but IMO it is too much noise and we should consider removing it.)Warning: Cannot update component ('BlockMobileToolbar) while rendering a different component ('SlotComponent').` (Discussed on internal channels.)Non-serialized values were found in navigation state. Check FocalPoint > params.onFocalPointChange(Navigating to nested Cover block bottom sheets logs warnings #30839)Clipboard has been extracted from react-native core and will be removed in a future release. It can now be installed from '@react-native-community/clipboard' instead of 'react-native'.when opening Image block > Settings.
| @@ -491,7 +491,7 @@ SPEC CHECKSUMS: | |||
| React-runtimeexecutor: cad74a1eaa53ee6e7a3620231939d8fe2c6afcf0 | |||
| ReactCommon: cfe2b7fd20e0dbd2d1185cd7d8f99633fbc5ff05 | |||
| ReactNativeDarkMode: 6d807bc8373b872472c8541fc3341817d979a6fb | |||
| RNCMaskedView: dfeba59697c44d36abec79c062aeb1ea343d610d | |||
| RNCMaskedView: 66caacf33c86eaa7d22b43178dd998257d5c2e4d | |||
There was a problem hiding this comment.
Strangely enough, these are the changes I see when running npm run native ios within this branch. I'm not sure which are correct. Could ruby or bundler versions be causing divergence? I actually see this type of diff quite often in this file, but have yet to explore why they occur.
❯ ruby -v
ruby 2.6.4p104 (2019-08-28 revision 67798) [x86_64-darwin20]
❯ bundle --version
Bundler version 2.2.14
There was a problem hiding this comment.
Aha, my versions are:
$ruby --version
ruby 2.6.0p0 (2018-12-25 revision 66547) [x86_64-darwin20]
$ bundle --version
Bundler version 2.2.16
@ceyhun , can this by any chance be related to having to use https://github.com/wordpress-mobile/gutenberg-mobile/blob/develop/bin/generate-podspecs.sh?
There was a problem hiding this comment.
react-native-modal doesn't seem to have any .podspec or any native code at all, so running generate-podspecs.sh shouldn't be necessary.
Seems like the checksums of packages/react-native-editor/ios/Pods/Local Podspecs/<package-name>.podspec.json are different for each of us.
Can you for example maybe share your react-native-video.podspec.json file in that folder, so we can compare?
Mine is:
react-native-video.podspec.json
{
"name": "react-native-video",
"version": "5.0.2",
"summary": "A <Video /> element for react-native",
"description": "A <Video /> element for react-native",
"license": "MIT",
"authors": {
"name": "Brent Vatne",
"email": "brentvatne@gmail.com",
"url": "https://github.com/brentvatne"
},
"homepage": "https://github.com/brentvatne/react-native-video",
"source": {
"git": "https://github.com/brentvatne/react-native-video.git",
"tag": "5.0.2"
},
"platforms": {
"ios": "8.0",
"tvos": "9.0"
},
"static_framework": true,
"dependencies": {
"React-Core": [
]
},
"default_subspecs": "Video",
"subspecs": [
{
"name": "Video",
"source_files": "ios/Video/*.{h,m}"
},
{
"name": "VideoCaching",
"dependencies": {
"react-native-video/Video": [
],
"SPTPersistentCache": [
"~> 1.1.0"
],
"DVAssetLoaderDelegate": [
"~> 0.3.1"
]
},
"source_files": "ios/VideoCaching/**/*.{h,m}"
}
]
}There was a problem hiding this comment.
(note: both files pasted below include an empty line at the end)
Here's mine (I'm including the full path name for clarity)
gutenberg-mobile-root/gutenberg/packages/react-native-editor/ios/Pods/Local Podspecs/react-native-video.podspec.json
{
"name": "react-native-video",
"version": "5.0.2",
"summary": "A <Video /> element for react-native",
"description": "A <Video /> element for react-native",
"license": "MIT",
"authors": "Brent Vatne <brentvatne@gmail.com> (https://github.com/brentvatne)",
"homepage": "https://github.com/brentvatne/react-native-video",
"source": {
"git": "https://github.com/brentvatne/react-native-video.git",
"tag": "5.0.2"
},
"platforms": {
"ios": "8.0",
"tvos": "9.0"
},
"static_framework": true,
"dependencies": {
"React-Core": [
]
},
"default_subspecs": "Video",
"subspecs": [
{
"name": "Video",
"source_files": "ios/Video/*.{h,m}"
},
{
"name": "VideoCaching",
"dependencies": {
"react-native-video/Video": [
],
"SPTPersistentCache": [
"~> 1.1.0"
],
"DVAssetLoaderDelegate": [
"~> 0.3.1"
]
},
"source_files": "ios/VideoCaching/**/*.{h,m}"
}
]
}
The other place I find such a file is:
gutenberg-mobile-root/third-party-podspecs/react-native-video.podspec.json
{
"name": "react-native-video",
"version": "5.0.2",
"summary": "A <Video /> element for react-native",
"description": "A <Video /> element for react-native",
"license": "MIT",
"authors": "Brent Vatne <brentvatne@gmail.com> (https://github.com/brentvatne)",
"homepage": "https://github.com/brentvatne/react-native-video",
"source": {
"git": "https://github.com/brentvatne/react-native-video.git",
"tag": "5.0.2"
},
"platforms": {
"ios": "8.0",
"tvos": "9.0"
},
"static_framework": true,
"dependencies": {
"React-Core": []
},
"default_subspecs": "Video",
"subspecs": [
{
"name": "Video",
"source_files": "ios/Video/*.{h,m}"
},
{
"name": "VideoCaching",
"dependencies": {
"react-native-video/Video": [],
"SPTPersistentCache": [
"~> 1.1.0"
],
"DVAssetLoaderDelegate": [
"~> 0.3.1"
]
},
"source_files": "ios/VideoCaching/**/*.{h,m}"
}
],
"__WARNING!__": "This file is autogenerated by generate-podspecs.sh script. Do not modify manually. Re-run the script if necessary."
}
In the meantime, I've reverted the Podfile.lock changes so we can merge the rest of the fixes.
There was a problem hiding this comment.
Here is my copy of packages/react-native-editor/ios/Pods/Local\ Podspecs/react-native-video.podspec.json.
react-native-video.podspec.json
{
"name": "react-native-video",
"version": "5.0.2",
"summary": "A <Video /> element for react-native",
"description": "A <Video /> element for react-native",
"license": "MIT",
"authors": {
"name": "Brent Vatne",
"email": "brentvatne@gmail.com",
"url": "https://github.com/brentvatne"
},
"homepage": "https://github.com/brentvatne/react-native-video",
"source": {
"git": "https://github.com/brentvatne/react-native-video.git",
"tag": "5.0.2"
},
"platforms": {
"ios": "8.0",
"tvos": "9.0"
},
"static_framework": true,
"dependencies": {
"React-Core": [
]
},
"default_subspecs": "Video",
"subspecs": [
{
"name": "Video",
"source_files": "ios/Video/*.{h,m}"
},
{
"name": "VideoCaching",
"dependencies": {
"react-native-video/Video": [
],
"SPTPersistentCache": [
"~> 1.1.0"
],
"DVAssetLoaderDelegate": [
"~> 0.3.1"
]
},
"source_files": "ios/VideoCaching/**/*.{h,m}"
}
]
}There was a problem hiding this comment.
did:
$ rm -Rf gutenberg/packages/react-native-editor/ios/Pods
$ rm -Rf gutenberg/packages/react-native-editor/ios/vendor
$ npm run core ios
but I get the exact diff as in the commit I committed originally 😬
There was a problem hiding this comment.
After pulling the latest and within gutenberg, running the following resulted in one change in the Podfile.lock for me.
rm -rf packages/react-native-editor/ios/Pods packages/react-native-editor/iOS/vendor
npm run core ios
There was a problem hiding this comment.
@hypest That's strange 🤔 Can you try one last thing maybe before removing the folders running in gutenberg/packages/react-native-editor/ios:
git checkout origin/trunk Podfile.lock
bundle exec pod cache clean --all
rm -rf Pods/
rm -rf vendor/
bundle install
bundle exec pod repo remove trunk
bundle exec pod install --repo-update@dcalhoun Can you maybe share the gutenberg/packages/react-native-editor/ios/Pods/Local Podspecs/FBReactNativeSpec.podspec.json file? And maybe I guess trying the above as well won't hurt :)
There was a problem hiding this comment.
Here are the contents of my FBReactNativeSpec.podspec.json.
packages/react-native-editor/ios/Pods/Local\ Podspecs/FBReactNativeSpec.podspec.json
{
"name": "FBReactNativeSpec",
"version": "0.64.0",
"summary": "-",
"homepage": "https://reactnative.dev/",
"license": "MIT",
"authors": "Facebook, Inc. and its affiliates",
"platforms": {
"ios": "10.0"
},
"compiler_flags": "-DFOLLY_NO_CONFIG -DFOLLY_MOBILE=1 -DFOLLY_USE_LIBCPP=1 -Wno-comma -Wno-shorten-64-to-32 -Wno-nullability-completeness",
"source": {
"git": "https://github.com/facebook/react-native.git",
"tag": "v0.64.0"
},
"source_files": "**/*.{c,h,m,mm,cpp}",
"header_dir": "FBReactNativeSpec",
"pod_target_xcconfig": {
"USE_HEADERMAP": "YES",
"CLANG_CXX_LANGUAGE_STANDARD": "c++14",
"HEADER_SEARCH_PATHS": "\"$(PODS_TARGET_SRCROOT)/React/FBReactNativeSpec\" \"$(PODS_ROOT)/RCT-Folly\""
},
"dependencies": {
"RCT-Folly": [
"2020.01.13.00"
],
"RCTRequired": [
"0.64.0"
],
"RCTTypeSafety": [
"0.64.0"
],
"React-Core": [
"0.64.0"
],
"React-jsi": [
"0.64.0"
],
"ReactCommon/turbomodule/core": [
"0.64.0"
]
},
"script_phases": {
"name": "Generate Specs",
"input_files": [
"/Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../Libraries"
],
"output_files": [
"$(DERIVED_FILE_DIR)/codegen-FBReactNativeSpec.log",
"/Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../React/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h",
"/Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../React/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec-generated.mm"
],
"script": "set -o pipefail\n\nbash -l -c 'SRCS_DIR=/Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../Libraries CODEGEN_MODULES_OUTPUT_DIR=/Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../React/FBReactNativeSpec/FBReactNativeSpec CODEGEN_MODULES_LIBRARY_NAME=FBReactNativeSpec /Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/generate-specs.sh' 2>&1 | tee \"${SCRIPT_OUTPUT_FILE_0}\"",
"execution_position": "before_compile",
"show_env_vars_in_log": true
},
"prepare_command": "mkdir -p /Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../React/FBReactNativeSpec/FBReactNativeSpec && touch /Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../React/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec.h /Users/davidcalhoun/Sites/a8c/gutenberg/node_modules/react-native/scripts/../React/FBReactNativeSpec/FBReactNativeSpec/FBReactNativeSpec-generated.mm"
}I ran the step you provided in your last comment and saw the same changes afterwards:
There was a problem hiding this comment.
Thanks @dcalhoun! Looks like the paths in the script_phases is different. And I guess it will be different for everyone because they are dynamically generated via this in here.
This phase makes the iOS build call this script each time during a build. I've already added a hack in gutenberg-mobile to remove this script_phases when we generate the static podspecs to be used by WPiOS here and hosted those generated files instead on gutenberg-mobile here.
I guess we can do a similar thing in Gutenberg demo app's Podfile somewhere here and maybe even host them in Gutenberg instead of Gutenberg Mobile for both Gutenberg Demo app and WPiOS 🤔
This reverts commit ce1baea.
Issue message: "Error: EISDIR: illegal operation on a directory, read" Fix PR on Metro: facebook/metro#567
hypest
changed the title
|
Thanks for reviewing @dcalhoun !
Good catch! Fixed with 9871a34.
Yeap, that's something we should improve at some point 😭
Tracked that to this validation logic and am not sure yet what to do about it. We'll prolly handle it in a separate PR, wdyt?
Agree, it's too noisy at this point and we should do something about it. Just removing it feels not enough as we probably want to track it or fix it but yeah, a story for a separate PR I guess.
@fluiddot is attempting a fix for that, separately.
Hmm, a fix doesn't quickly occur to me, wdyt about handling it in a separate PR?
Aha. Looks like indeed, we need to migrate to the extracted Clipboard package. It seems that it won't be too hard but it needs some native side linking so, wdyt about doing that in a separate PR? |
@hypest agreed that all of these can be addressed in a separate PR. |
|
Do you happen to get the following busy log too @dcalhoun perhaps? |
@hypest I presume you meant "build log?" If so, I have seen that warning before — I believe during |
I get lots of those during the Metro bundling, after Anyway, let's merge this one and we can wrangle that separately too. Edit: PR here. |
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.
Bumping the
react-native-modalversion we use to fix the "Animated.event now requires a second argument for options" log caused by using a fairly old version of the lib.Took the opportunity to fix more warnings too, see the "Changes" list below.
gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#3630
How has this been tested?
Types of changes
Changes
react-native-modalto remove the "Animated.event now requires a second argument for options" message in the logs when the app starts.Modal.onSwipeand start usingModal.onSwipeCompleteas per the message.AccessibilityInfo.fetchand start usingAccessibilityInfo.isScreenReaderEnabledas per this note.componentWillUnmountwithcomponentDidMount1. Ensured that the original fix still works.react-native-editor/Podfile.lockgot updated when I rannpm run core iosin gutenberg-mobile, not sure if the changes there are warranted 🤔Toolbarinstances as that format is deprecated. UsedToolbarGroupinstead, as recommended by the doc. Seems to work fine, while my attempt to add labels actually led to the keyboard dismiss button to get misplaced.Checklist:
*.native.jsfiles for terms that need renaming or removal).