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

feat: make RCTBlobManager TurboModule-compatible #35047

Closed
wants to merge 1 commit into from

Conversation

andrestone
Copy link
Contributor

@andrestone andrestone commented Oct 21, 2022

Summary

Currently, RCTBlobManager (the native module for Blob support) cannot be loaded on iOS when the new architecture is enabled.

Changelog

[General] [Added] - BlobModule to RCTCoreModulesClassProvider

Test Plan

The snippet below can be used to test Blob support with the new architecture enabled.

// App.tsx
import { useEffect } from 'react';
import { View } from 'react-native';
function uriToBlob(uri: any) {
  return new Promise((resolve, reject) => {
    const xhr = new XMLHttpRequest();
    xhr.responseType = 'blob';
    xhr.onload = () => {
      const blob = xhr.response;
      resolve(blob);
    };
    xhr.onerror = err => {
      reject(err);
    };
    xhr.open('GET', uri);
    xhr.send();
  });
}

export default function App() {
  useEffect(() => {
    uriToBlob('https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png');
  });
  return <View />;
}

Related issue: #35042

@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Oct 21, 2022
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,755,795 +0
android hermes armeabi-v7a 7,159,884 +0
android hermes x86 8,067,403 +0
android hermes x86_64 8,038,126 +0
android jsc arm64-v8a 9,613,996 +0
android jsc armeabi-v7a 8,379,971 +0
android jsc x86 9,560,820 +0
android jsc x86_64 10,153,787 +0

Base commit: 48db6be
Branch: main

@analysis-bot
Copy link

analysis-bot commented Oct 21, 2022

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

Base commit: fd91748
Branch: main

@pull-bot
Copy link

PR build artifact for 1964d07 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 1964d07 is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@react-native-bot react-native-bot added the No CLA Authors need to sign the CLA before a PR can be reviewed. label Oct 22, 2022
@cortinico cortinico requested a review from RSNara October 24, 2022 17:45
@cortinico
Copy link
Contributor

@RSNara can you review this one?
@andrestone can you accept the CLA?

@andrestone
Copy link
Contributor Author

andrestone commented Oct 24, 2022

@andrestone can you accept the CLA?

Done. Thanks.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 24, 2022
@react-native-bot react-native-bot removed the No CLA Authors need to sign the CLA before a PR can be reviewed. label Oct 24, 2022
@cipolleschi
Copy link
Contributor

/rebase

@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@pull-bot
Copy link

PR build artifact for 6269a1e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@pull-bot
Copy link

PR build artifact for 6269a1e is ready.
To use, download tarball from "Artifacts" tab in this CircleCI job then run yarn add <path to tarball> in your React Native project.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @andrestone in 279cfec.

When will my fix make it into a release? | Upcoming Releases

@cipolleschi
Copy link
Contributor

Hi! In the context of cutting the branch for 0.71.0-rc0, we had to revert this commit in #35188, for 2 reasons:

  1. It was breaking RNTester. If you run RNTester with the New Architecture and you navigate to the Image component, you will see an error when the ImageLoader tries to fetch an image from the Bundle
  2. These two files are automatically generated by a script. Whenever we run the script internally, those change will be gone. (As highlighted by Line 7 of these files)

We will look into this thing before releasing 0.71 to make sure that the RCTBlobManager works properly!

dmytrorykun added a commit to dmytrorykun/react-native that referenced this pull request Nov 3, 2022
Summary:
This diff reverts D40716048 (facebook@279cfec) (facebook#35047) which breaks RCTImageLoader.
https://pxl.cl/2jKrM
Those files are auto-generated and are not supposed to be edited manually.
Changelog: [iOS] [Fixed] - facebook#35047 reverted.

Reviewed By: cipolleschi

Differential Revision: D40979350

fbshipit-source-id: ece2b9c653fe01e209a523e6a99e41a0605fddac
facebook-github-bot pushed a commit that referenced this pull request Nov 3, 2022
Summary:
Pull Request resolved: #35188

This diff reverts D40716048 (279cfec) (#35047) which breaks RCTImageLoader.
https://pxl.cl/2jKrM
Those files are auto-generated and are not supposed to be edited manually.
Changelog: [iOS] [Fixed] - #35047 reverted.

Reviewed By: cipolleschi

Differential Revision: D40979350

fbshipit-source-id: ef92cf05636cba818151d4184e0275a3aab56cff
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Currently, RCTBlobManager (the native module for Blob support) cannot be loaded on iOS when the new architecture is enabled.

## Changelog

[General] [Added] - `BlobModule` to `RCTCoreModulesClassProvider`

Pull Request resolved: facebook#35047

Test Plan:
The snippet below can be used to test Blob support with the new architecture enabled.

```
// App.tsx
import { useEffect } from 'react';
import { View } from 'react-native';
function uriToBlob(uri: any) {
  return new Promise((resolve, reject) => {
    const xhr = new XMLHttpRequest();
    xhr.responseType = 'blob';
    xhr.onload = () => {
      const blob = xhr.response;
      resolve(blob);
    };
    xhr.onerror = err => {
      reject(err);
    };
    xhr.open('GET', uri);
    xhr.send();
  });
}

export default function App() {
  useEffect(() => {
    uriToBlob('https://www.google.com/images/branding/googlelogo/1x/googlelogo_color_272x92dp.png');
  });
  return <View />;
}

```

Related issue: facebook#35042

Reviewed By: NickGerleman

Differential Revision: D40716048

Pulled By: cipolleschi

fbshipit-source-id: 17643d230fa7ea83baee363d137d51f87818baa8
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
Pull Request resolved: facebook#35188

This diff reverts D40716048 (facebook@279cfec) (facebook#35047) which breaks RCTImageLoader.
https://pxl.cl/2jKrM
Those files are auto-generated and are not supposed to be edited manually.
Changelog: [iOS] [Fixed] - facebook#35047 reverted.

Reviewed By: cipolleschi

Differential Revision: D40979350

fbshipit-source-id: ef92cf05636cba818151d4184e0275a3aab56cff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants