Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Gutenberg] Upgrade React Native 0.71.11 #20956

Merged
merged 57 commits into from
Jul 27, 2023

Conversation

SiobhyB
Copy link
Contributor

@SiobhyB SiobhyB commented Jun 26, 2023

Related to wordpress-mobile/gutenberg-mobile#5874

This PR contains the following changes:

  • Bump Gutenberg Mobile reference to the latest commit of GB-mobile branch upgrade/react-native-0.71.8. This incorporates the needed changes for the RN upgrade coming from Gutenberg.
  • Integration of Gutenberg using XCFramework (further information can be found in this PR).
  • Fix for failing UI tests caused by the RN upgrade (further information can be found in this PR).

To test:
Follow testing instructions from WordPress/gutenberg#51303.

Regression Notes

  1. Potential unintended areas of impact
    None apart from the editor.

  2. What I did to test those areas of impact (or what existing automated tests I relied on)
    Manual testing and CI tests.

  3. What automated tests I added (or what prevented me from doing so)
    N/A

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

UI Changes testing checklist:

  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

mokagio and others added 22 commits July 3, 2023 15:46
No other than reason than me looking at the CocoaPods setup and
realizing we were a few versions behind.
Despite it working during the prototype stage, I wasn't able to get the
XCFrameworks out of the ZIP archive, but it works fine with the tar.gz.

I suspect this has to do with the folder(s) generated when
decompressing.
These are unnecessary at the moment but will be when we move to
Gutenberg via XCFramework
The advantage of this approach is that downloading the archive is only
necessary when using a local spec. Defining the logic in the local spec
itself keeps everything self contained and saves us from having to
conditionally call the `pre_install` hook.
The idea was to use a local spec and interpolated the desired commit
SHA1 in the `source` to download the corresponding `tar.gz`.

However, CocoaPods has some issues with local specs that use `http`
`source`, as documented in:

- CocoaPods/CocoaPods#10288 (comment)
- https://github.com/firebase/firebase-ios-sdk/blob/68b39b8edf61f6e643e2396e712c7c67e0f146ff/scripts/pod_lib_lint.rb#L70-L78

Using a remote spec doesn't have the same issue, and the cost in terms
of extra computation and storage is negligible when compared to building
and hosting the `tar.gz` archives.
This was done to address the following CI failure when building with the
Gutenberg XCFramework:

```
▸ Linking WordPress
⚠️  ld: Could not find or use auto-linked library 'swiftCompatibility56'

❌  ld: symbol(s) not found for architecture x86_64
```

https://buildkite.com/automattic/wordpress-ios/builds/14356#01885545-c05a-43a8-b475-d0d683857672
Because the latest XCFramework setup ships without testing compilation.
Co-authored-by: Tony Li <tony.li@automattic.com>
Co-authored-by: Tony Li <tony.li@automattic.com>
Copy link
Contributor

@fluiddot fluiddot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎊 !

I shared some comments but none should be blocking the PR.

Comment on lines +67 to +68
# When referencing via a tag or commit, we download pre-built frameworks.
return if options.key?(:tag) || options.key?(:commit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These conditions will never be met as gutenberg_dependencies is only called when using Gutenberg locally, i.e. it will always reference the pod using a path.

Copy link
Contributor

@fluiddot fluiddot Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it makes sense to keep it in case gutenberg_dependencies is used somewhere else in the future 🤔.

Gutenberg/cocoapods_helpers.rb Outdated Show resolved Hide resolved
Comment on lines 117 to 122
# When using an absolute path, we get this error, notice the duplicated "/Users/gio/Developer/a8c/wpios":
#
# [!] An error occurred while processing the post-install hook of the Podfile.
#
# No such file or directory @ rb_sysopen - <GUTENBERG_MOBILE_PROJECT_PATH>/gutenberg/node_modules/react-native/package.json
#
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we confirmed that React Native preprends $PWD, I think we don't need to share details about the error. Hence, we could consider removing this part.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've applied this suggestion via 7e11b1e.

Scripts/BuildPhases/CopyGutenbergJS.sh Outdated Show resolved Hide resolved
Comment on lines +658 to +659
@unknown default:
fatalError()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, when building the app using a local installation of Gutenberg, this line (and similar ones below) are giving the following warning when building: Default will never be executed

However, this is not the case when building using the XCFramework 🤔

@fluiddot
Copy link
Contributor

The CI jobs are failing with the following error after incorporating the latest changes from trunk:

❌ wordpress-ios/WordPress/Classes/ViewRelated/Media/MemoryCache.swift:4:8: no such module 'SDWebImage'
--
  | import SDWebImage
  | ^

The failure is related to this PR and the usage of the library SDWebImage (reference). This library is a transitive dependency of the RNFastImage library, which is only used by the Gutenberg editor.


WordPress-iOS/Podfile.lock

Lines 447 to 449 in eb05889

- RNFastImage (8.5.11):
- React-Core
- SDWebImage (~> 5.11.1)

Now that Gutenberg is integrated via XCFramework, the RNFastImage library is no longer a dependency in the project (reference). Hence, we have to either re-incorporate it or stop using it. For now, I'll add it to the Podfile (131057d) as I presume we need it but I'd appreciate feedback from the developers involved in the PR to confirm it (cc @kean @staskus @guarani). Thanks for the help 🙇 !

@fluiddot fluiddot merged commit 1614edc into trunk Jul 27, 2023
@fluiddot fluiddot deleted the gutenberg/upgrade/react-native-0.71.11 branch July 27, 2023 15:38
guarani added a commit that referenced this pull request Aug 8, 2023
This proposes replaces usage of `SDWebImage` in the new `MemoryCache` following the comment #20956 (comment). It's only being used in one instance, to calculate the cost of objects added to the cache.

What's the difference between not passing in a cost and using `sd_memoryCost`? I'm not sure, so I'll share my current understanding and ask for feedback. For images, I don't see a difference because I think the cost is the image's size in bytes. For gifs (also `UIImage`s), the cost calculation seems more complex since I think it's only the number of frames loaded into memory (not the frames that are kept on disk).

This PR is meant to start a conversation about which option to take:
a. Let `NSCache` calculate the cost and remove `SDWebImage` from the Podfile
b. Re-implement a cost calculation and remove `SDWebImage` from the Podfile
c. Leave things as-is and close this PR

The assumption here is that by removing `SDWebImage`, we're not importing the library twice (once here and once in Gutenberg as a transitive dependency of a RN library).
@guarani guarani mentioned this pull request Aug 8, 2023
13 tasks
kean pushed a commit that referenced this pull request Nov 20, 2023
This proposes replaces usage of `SDWebImage` in the new `MemoryCache` following the comment #20956 (comment). It's only being used in one instance, to calculate the cost of objects added to the cache.

What's the difference between not passing in a cost and using `sd_memoryCost`? I'm not sure, so I'll share my current understanding and ask for feedback. For images, I don't see a difference because I think the cost is the image's size in bytes. For gifs (also `UIImage`s), the cost calculation seems more complex since I think it's only the number of frames loaded into memory (not the frames that are kept on disk).

This PR is meant to start a conversation about which option to take:
a. Let `NSCache` calculate the cost and remove `SDWebImage` from the Podfile
b. Re-implement a cost calculation and remove `SDWebImage` from the Podfile
c. Leave things as-is and close this PR

The assumption here is that by removing `SDWebImage`, we're not importing the library twice (once here and once in Gutenberg as a transitive dependency of a RN library).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Gutenberg Editing and display of Gutenberg blocks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants