Image: support ImageSource with headers #2442

kidroca · 2022-11-25T13:29:38Z

Details

Extend ImageLoader functionality to be able to work with image sources containing headers

We preserve the existing strategy that works with image.src for cases where source
is just an uri with no headers

When source contain headers we make a fetch request and then render a local url for the
downloaded blob (URL.createObjectURL)

Fixed Issues

fixes Image: support HTTP properties for 'source' objects #1019

Test Strategy

Verify existing Image functionality has no regressions
- build web and examples: npm run dev -w react-native-web and npm run dev -w react-native-web-examples
- open the examples page and go to Image: http://localhost:3000/image
- see images are loading
- take a screenshot and do the same from the master branch. You can switch back and forth and verify the image are loading the same way
Verify Images with headers can be loaded
- build web and examples: npm run dev -w react-native-web and npm run dev -w react-native-web-examples
- open the examples page and go to Image: http://localhost:3000/image
- modify sourceWithHeaders here packages/react-native-web-examples/pages/image/index.js and try to load images from a server that expects a GET request with a header
- verify the image is loading on the examples page (near the bottom, labeled: "With Headers")

Example Project Screenshot

The code is also tested in a standalone project and it loads images with headers successfully

codesandbox-ci · 2022-11-25T13:30:39Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 616975e:

Sandbox	Source
react-native-web-examples	Configuration

kidroca

Leaving some notes on the code changes

There's a breaking change that might be introduced or reverted - it's regarding the onLoad prop and the parameter structure used - I've opened a thread on the relevant source code chunk

packages/react-native-web/src/modules/ImageLoader/index.js

packages/react-native-web/src/exports/Image/__tests__/__snapshots__/index-test.js.snap

packages/react-native-web/src/exports/Image/__tests__/index-test.js

packages/react-native-web/src/exports/Image/index.js

packages/react-native-web/src/modules/ImageLoader/index.js

necolas · 2022-11-28T19:47:21Z

It would have been a good idea to ask about this before writing the PR, as there are numerous other plans around Image.

There's this PR for crossOrigin support #2207

This milestone https://github.com/necolas/react-native-web/milestone/18

And a parallel effort to support W3C props on Image facebook/react-native#34424

kidroca · 2022-11-29T14:44:59Z

Hi @necolas
I thought this PR would be a nice place to get the conversation going

This PR is following the original idea to support headers via XHR, though I've opted to use fetch since the minimal browser requirements allow it

Overall I think the changes here are compatible with #2207

A big chunk of the changes comes from added tests
And the change for handling source objects in hook dependencies, which is also a problem for #2207 - https://github.com/necolas/react-native-web/pull/2207/files#diff-7cb74a3a32d73857be80350ecd1ea131d256bd5af11d2000e4fc2d03c2230584R222-R223

The rest of the changes are small cleanups and moving existing logic - I can revert some of these namely resolveAssetUri to avoid conflicts with the other PR

If there are any problems, I'm happy to discuss and modify the PR

necolas · 2022-11-30T05:16:10Z

Please resolve the conflicts with master

kidroca · 2022-11-30T18:41:26Z

There are no conflicts with master right now

There would be conflicts for the PR that happens to be merged last due to the changes to resolveAssetUri I made

Since the other PR just moves the function without changing the logic, it might be easier to do the same after this PR is merged
Alternatively I can apply the change after the other PR is merged

necolas · 2022-11-30T20:26:19Z

Here's what GitHub shows me on this PR

necolas · 2022-11-30T20:27:36Z

If you could rebase/squash your 20 commits into just a few, that would help too. Thanks

kidroca

Squashed existing commits to allow rebasing

packages/react-native-web/src/exports/Image/__tests__/index-test.js

kidroca

PR Updated using an alternative strategy to load images with headers resulting in less changes overall

When a source with headers is used, we use a (kind of) a HOC component that fetches the source and then passes a local uri created with URL.createObjectURL to the original Image component

kidroca · 2023-01-17T11:12:13Z

packages/react-native-web/src/modules/ImageLoader/index.js

+        nativeEvent.source = {
+          uri: image.src,
+          width: image.naturalWidth,
+          height: image.naturalHeight
+        };


Attach source to nativeEvent so onLoad matches the RN Image interface https://reactnative.dev/docs/image#onload

packages/react-native-web/src/exports/Image/index.js

Beamanator · 2023-01-18T09:39:34Z

@necolas any chance you or a fellow maintainer can check out these changes? We're fine to wait since, as you mentioned before, you have many updates planned - we're just hoping to get a possible timeline when this could get reviewed.

necolas · 2023-01-18T22:35:18Z

It looks like the author is still working on the patch. It needs unit tests and the suggestion to make changes to make testing easier makes sense to me.

necolas · 2023-01-30T18:41:27Z

What's next for this PR? The PR in the fork had no new unit tests and this one hasn't been updated for 2 weeks

kidroca · 2023-01-31T09:53:44Z

I haven't synced my latest changes here, and I still need to write some unit tests
I'll post an update when I'm done

kidroca

Ready for Review ✅

kidroca · 2023-02-03T13:32:38Z

packages/react-native-web/src/exports/Image/__tests__/index-test.js

-      jest.useFakeTimers();
-      ImageLoader.load = jest.fn().mockImplementation((_, onLoad, onError) => {
-        onLoad();
-      });


Fake timers no longer needed to pass this test

Actually the fake timers are breaking the (new) tests that use Promises

For some reason unless we remove all calls to jest.useFakeTimers() - return waitFor(() => ...) does not work

kidroca · 2023-02-03T13:34:45Z

packages/react-native-web/src/exports/Image/index.js

+function raiseOnErrorEvent(uri, { onError, onLoadEnd }) {
+  if (onError) {
+    onError({
+      nativeEvent: {
+        error: `Failed to load resource ${uri} (404)`
+      }
+    });
+  }
+  if (onLoadEnd) onLoadEnd();
+}


This was extracted for reuse out of the original Image component hook
(We reuse this in case loading with headers fails)

kidroca · 2023-02-03T13:37:27Z

packages/react-native-web/src/exports/Image/index.js

+/**
+ * This component handles specifically loading an image source with headers
+ * default source is never loaded using headers
+ */
+const ImageWithHeaders: ImageComponent = React.forwardRef((props, ref) => {
+  // $FlowIgnore: This component would only be rendered when `source` matches `ImageSource`
+  const nextSource: ImageSource = props.source;


Perhaps there's a way to declare ImageWithHeaders as an Image component where the source prop is exactly type ImageSource, but I don't know exactly how

I guess I need to declare ImageWithHeaderProp type that spreads the default Image prop type but changes the source. Should I do that?

kidroca · 2023-02-03T13:39:05Z

packages/react-native-web/src/exports/Image/types.js

+export type ImageProps = {|
+  ...$Exact<ViewProps>,


I had Flow errors after my changes
Following the error messages and suggested fixes led to these changes

kidroca · 2023-02-03T13:39:24Z

packages/react-native-web/src/exports/ImageBackground/index.js

-type ImageBackgroundProps = {
+type ImageBackgroundProps = {|
  ...ImageProps,
  imageRef?: any,
  imageStyle?: $PropertyType<ImageProps, 'style'>,
  style?: $PropertyType<ViewProps, 'style'>
-};
+|};


I had Flow errors after my changes
Following the error messages and suggested fixes led to these changes

necolas · 2023-03-29T18:38:49Z

Hi, please could you rebase this. Now that 0.19 is done, the 0.20 release will be focused on Image changes.

kidroca · 2023-04-04T07:15:49Z

✅ Rebased

necolas · 2023-04-10T20:02:03Z

packages/react-native-web/src/exports/Image/index.js

+  };
+
+  return <BaseImage ref={ref} {...propsToPass} />;
+});

 // $FlowIgnore: This is the correct type, but casting makes it unhappy since the variables aren't defined yet


When this old $FlowIgnore is removed, there is a new type error:

Cannot assign React.forwardRef(...) to ImageWithStatics because property getSize is missing in AbstractComponent [1] but exists in ImageStatics [2]. [incompatible-type]

I think I've seen the original error the comment references at some point
Could it be the error was fixed by a flow version update, but now we have this other error?

Extend ImageLoader functionality to be able to work with image sources containing headers We preserve the existing strategy that works with image.src for cases where source is just an uri with no headers When sources contain headers we make a fetch request and then render a local url for the downloaded blob (URL.createObjectURL) Fix #1019 Close #2442

necolas · 2023-04-10T20:35:38Z

This PR was merged into the 0.20-dev branch #2508

Extend ImageLoader functionality to be able to work with image sources containing headers We preserve the existing strategy that works with image.src for cases where source is just an uri with no headers When sources contain headers we make a fetch request and then render a local url for the downloaded blob (URL.createObjectURL) Fix #1019 Fix #2268 Close #2442

kidroca commented Nov 25, 2022

View reviewed changes

kidroca marked this pull request as ready for review November 25, 2022 13:59

Beamanator mentioned this pull request Nov 29, 2022

[Tracking][Image][Polish] Image-related feature request and improvements Expensify/App#10894

Closed

39 tasks

kidroca mentioned this pull request Dec 1, 2022

Image updates: remove BG-style, add crossOrigin functionality, allow nullable alt text #2207

Open

kidroca force-pushed the kidroca/feat/image-source-headers branch 2 times, most recently from 27f7163 to 7ecabef Compare December 1, 2022 20:15

kidroca commented Dec 1, 2022

View reviewed changes

packages/react-native-web/src/exports/Image/__tests__/index-test.js Outdated Show resolved Hide resolved

kidroca force-pushed the kidroca/feat/image-source-headers branch from 7ecabef to 8324905 Compare December 1, 2022 20:23

Beamanator mentioned this pull request Dec 2, 2022

Image Web/Desktop: Add support for http headers Expensify/App#13036

Merged

56 tasks

This comment was marked as off-topic.

Sign in to view

kidroca force-pushed the kidroca/feat/image-source-headers branch from 8324905 to 2d82778 Compare January 17, 2023 11:08

kidroca commented Jan 17, 2023

View reviewed changes

kidroca mentioned this pull request Jan 19, 2023

[add] Image source headers handling Expensify/react-native-web#11

Merged

Beamanator mentioned this pull request Jan 31, 2023

Undo fork from #11463 Expensify/App#13846

Closed

kidroca force-pushed the kidroca/feat/image-source-headers branch 2 times, most recently from 8533f2d to 081216d Compare February 3, 2023 13:41

kidroca commented Feb 3, 2023

View reviewed changes

necolas added this to the 0.20: Image milestone Mar 20, 2023

kidroca added 3 commits April 4, 2023 10:01

[add] Image source headers handling

40eb370

[add] Image: Append source to nativeEvent

be12f9f

[test] Image: test headers related functionality

616975e

kidroca force-pushed the kidroca/feat/image-source-headers branch from 081216d to 616975e Compare April 4, 2023 07:06

raqso approved these changes Apr 4, 2023

View reviewed changes

necolas reviewed Apr 10, 2023

View reviewed changes

necolas added the has: pr Subject of a pull request label Apr 10, 2023

This was referenced Apr 10, 2023

added support for image source object #1843

Closed

fix: support image source objects #1470

Closed

roryabraham mentioned this pull request Nov 30, 2023

mWeb - The animation is not loading at all when use is offline Expensify/App#31236

Closed

6 tasks

Beamanator mentioned this pull request Dec 6, 2023

Tracker to follow up with react-native-web patches Expensify/App#32544

Closed

kidroca mentioned this pull request Jan 15, 2024

Image Web/Desktop: Add support for http headers Expensify/App#34505

Merged

48 tasks

kidroca mentioned this pull request Feb 15, 2024

Image Web/Desktop: Add support for http headers Expensify/App#36560

Merged

48 tasks

bernhardoj mentioned this pull request Jul 1, 2024

[Pay 7/30] [Dev] - [StrictMode] Attachments not loading and app crashes when click on loading preview from attachment Expensify/App#44566

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image: support ImageSource with headers #2442

Image: support ImageSource with headers #2442

kidroca commented Nov 25, 2022 •

edited

Loading

codesandbox-ci bot commented Nov 25, 2022 •

edited

Loading

kidroca left a comment

necolas commented Nov 28, 2022

kidroca commented Nov 29, 2022

necolas commented Nov 30, 2022

kidroca commented Nov 30, 2022

necolas commented Nov 30, 2022

necolas commented Nov 30, 2022

kidroca left a comment

This comment was marked as off-topic.

kidroca left a comment

kidroca Jan 17, 2023

Beamanator commented Jan 18, 2023

necolas commented Jan 18, 2023

necolas commented Jan 30, 2023

kidroca commented Jan 31, 2023

kidroca left a comment

kidroca Feb 3, 2023

kidroca Feb 3, 2023

kidroca Feb 3, 2023

kidroca Feb 3, 2023

kidroca Feb 3, 2023

kidroca Feb 3, 2023

necolas commented Mar 29, 2023

kidroca commented Apr 4, 2023

necolas Apr 10, 2023

kidroca Apr 10, 2023

necolas commented Apr 10, 2023

Image: support ImageSource with headers #2442

Are you sure you want to change the base?

Image: support ImageSource with headers #2442

Conversation

kidroca commented Nov 25, 2022 • edited Loading

Details

Fixed Issues

Test Strategy

codesandbox-ci bot commented Nov 25, 2022 • edited Loading

kidroca left a comment

Choose a reason for hiding this comment

necolas commented Nov 28, 2022

kidroca commented Nov 29, 2022

necolas commented Nov 30, 2022

kidroca commented Nov 30, 2022

necolas commented Nov 30, 2022

necolas commented Nov 30, 2022

kidroca left a comment

Choose a reason for hiding this comment

This comment was marked as off-topic.

kidroca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Beamanator commented Jan 18, 2023

necolas commented Jan 18, 2023

necolas commented Jan 30, 2023

kidroca commented Jan 31, 2023

kidroca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necolas commented Mar 29, 2023

kidroca commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

necolas commented Apr 10, 2023

kidroca commented Nov 25, 2022 •

edited

Loading

codesandbox-ci bot commented Nov 25, 2022 •

edited

Loading