Skip to content

Conversation

Saadnajmi
Copy link
Collaborator

@Saadnajmi Saadnajmi commented Jan 4, 2023

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

This came up during the 0.71 merge. There was an iOS only method we had already shimmed in RCTUIKit:

NSData *UIImagePNGRepresentation(NSImage *image) {
  return NSImageDataForFileType(image, NSBitmapImageFileTypePNG, @{});
}

NSData *UIImageJPEGRepresentation(NSImage *image, CGFloat compressionQuality) {
  return NSImageDataForFileType(image,
                                NSBitmapImageFileTypeJPEG,
                                @{NSImageCompressionFactor: @(1.0)});
}

As it turns out, we can't depend on RCTUIKit here, because this file's podspec (React-RCTImage) doesn't take a dependency on RCTUIKit's pod React-Core. The #import <React/RCTUIKit.h> most likely worked out of happenstance. I went with copying the methods needed from RCTUIKit to RCTImageLoader as we already had some duplication.

Let's also fix a bug where compressionQuality was hardcoded.

Changelog

[macOS] [Fixed] - Remove unnecessary diff in RCTImageLoader

Test Plan

CI should pass

@Saadnajmi Saadnajmi requested a review from a team as a code owner January 4, 2023 18:11
@Saadnajmi Saadnajmi added this to the 0.71 milestone Jan 4, 2023
@analysis-bot
Copy link

analysis-bot commented Jan 4, 2023

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 7222926
Branch: main

@Saadnajmi
Copy link
Collaborator Author

As it turns out, we can't depend on RCTUIKit here, because this file's podspec (React-RCTImage) doesn't take a dependency on RCTUIKit's pod React-Core. The #import <React/RCTUIKit.h> most likely worked out of happenstance.

I went with copying the methods needed over to the other file, as we already had some duplication. Let me know if there's an issue with this approach. I can also just close the PR and leave things as is.

@Saadnajmi Saadnajmi changed the title Remove unnecessary diff in RCTImageLoader Remove diff in RCTImageLoader Jan 5, 2023
@Saadnajmi Saadnajmi enabled auto-merge (squash) January 6, 2023 00:22
@Saadnajmi Saadnajmi merged commit 0ce8502 into microsoft:main Jan 6, 2023
@Saadnajmi Saadnajmi deleted the image-diff branch January 15, 2023 23:46
@Saadnajmi Saadnajmi self-assigned this Jan 16, 2023
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
* Remove unnecessary diff in RCTImageLoader

* Remove RCTUIKit dependency

* Fix bug where we hardcode compression quality

* Update comment

* Update RCTImageLoader.mm

* Update RCTImageLoader.mm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants