[add] Image source headers handling #11

kidroca · 2023-01-06T21:47:56Z

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

$ Expensify/App#12603

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")

kidroca · 2023-01-17T11:28:38Z

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

+    let uri;
+    const abortCtrl = new AbortController();
+    const request = new Request(nextSource.uri, {
+      headers: nextSource.headers,
+      signal: abortCtrl.signal
+    });
+    request.headers.append('accept', 'image/*');
+
+    if (onLoadStart) onLoadStart();
+
+    fetch(request)
+      .then((response) => response.blob())
+      .then((blob) => {
+        uri = URL.createObjectURL(blob);
+        setBlobUri(uri);
+      })
+      .catch((error) => {
+        if (error.name !== 'AbortError' && onError) {
+          onError({ nativeEvent: error.message });
+        }
+      });


I've tried to write some unit tests covering the new functionality, but because of this logic a few mocks need to be setup and make the PR bigger than it has to be

I think it might be better to move this logic to ImageLoader.loadUsingHeaders so in unit tests we can stub that method and ensure it gets called when we intent to

It's easy to manually verify the fetch logic works and downloads content
Once that's clear it's easy to just verify loadUsingHeaders is called correctly with expected input

I've made a similar comment on the main repo: https://github.com/necolas/react-native-web/pull/2442/files#r1072080652

What do you think, should we refactor / write some tests now or let the mainstream repo request those changes?

Hmm since this logic is the real meat & potatoes of the ImageWithHeaders component, if we move it to ImageLoader.loadUsingHeaders I assume we could get rid of this ImageWithHeaders component, and basically check if there's headers passed in the props - if yes, we call ImageLoader.loadUsingHeaders instead of ImageLoader.load here?

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

Lines 269 to 294 in bd1f7b8

requestRef.current = ImageLoader.load(

uri,

function load(e) {

updateState(LOADED);

if (onLoad) {

onLoad(e);

}

if (onLoadEnd) {

onLoadEnd();

}

},

function error() {

updateState(ERRORED);

if (onError) {

onError({

nativeEvent: {

error: `Failed to load resource ${uri} (404)`

}

});

}

if (onLoadEnd) {

onLoadEnd();

}

}

);

}

If that's what you're thinking, I honestly do think that logic is cleaner, even without needing to write tests for Expensify's case so I'd say the refactor sounds like a nice idea

Well, the idea is purely to ease mocking but I'll give your suggestion a try

Last PR tried to solve everything inside the same component but that resulted in more logic

What a component like ImageWithHeaders gives us is a guarantee that source would always be an object with headers

BTW there's feedback on the mainstream PR: necolas#2442 (comment) that we should write tests and thumbs up for extracting ImageLoader.loadUsingHeaders

I've tried to remove the ImageWithHeaders component and use either loadUsingHeaders or load functions here: https://github.com/Expensify/react-native-web/compare/master...kidroca:react-native-web:kidroca/feat/image-loader-headers-alt-2?diff=unified

But it results in a similar amount of changes and modifies some of the original logic (and seems harder to review)

because now the source loading useEffect needs to account for objects

load and loadWithHeaders need to work in a similar way in order to be interchangeable (so they were modified to return a request.cancel function)

We might merge load and loadUsingHeaders instead of testing which one to run, though this would be similar to the first PR were load handled loading images with and without headers

Thanks for doing that refactor! Personally I prefer the new changes because it keeps all the image loading logic in ImageLoader and there's minimal changes to Image/index.js - I don't see those new changes too difficult to review (though I am not sure where lastLoadedSource gets updated).

I'd say let's move forward with this last refactor, as I think it's pretty straightforward compared to passing / not passing specific props to the BaseImage component required in this PR

IMO the refactor is more of a proof that there are more "gymnastics" necessary in order to make this work by changing the original logic inside Image component

Original logic is changed, the cleanup logic is different, ImageLoader.load is changed, people, including the mainstream maintainer would have to inspect how these used to work and whether the change is suitable

IMO the mainstream maintainer already saw the update and is fine with just moving the fetch call to ImageLoader.loadUsingHeaders. I'm still trying different things, but the most straightforward PR so far seems to be the current one

There's also a big rework planned for the Image and the original loading logic would change, it would probably be best to not write stuff that depend on it (in the alt branch loadUsingHeaders depends on load)

Let me update the current PR with loadUsingHeaders extracted to ImageLoader and then we can make one final decision which set of changes would work best for us

IMO the mainstream maintainer already saw the update and is fine with just moving the fetch call to ImageLoader.loadUsingHeaders. I'm still trying different things, but the most straightforward PR so far seems to be the current one

Yeah this is one additional reason I really like this approach, even though there may be some additional changes in the upstream repo needed that we don't need at the moment in this fork 👍

Let me update the current PR with loadUsingHeaders extracted to ImageLoader and then we can make one final decision which set of changes would work best for us

That sounds perfect, thanks so much @kidroca 👍

kidroca · 2023-01-17T11:31:02Z

In need of some Review feedback here

cc @Beamanator @marcaaron @tgolen

Beamanator · 2023-01-17T16:39:40Z

Sorry I didn't have a chance to get to this today, @marcaaron do you mind giving this a first pass if you have time today? I will definitely review tomorrow no matter what 👍

marcaaron · 2023-01-17T18:12:01Z

Chatted 1:1 with @Beamanator. I'd love to see a good faith effort on this one before chiming in. It sounds like there are some higher priority things on Alex's plate that kept him from getting to this today. Taking that information... I've determined that my review is not urgently needed here and Alex has agreed to give a first review (I think).

Beamanator

As a whole I'd say these changes were much easy to follow, thanks for this PR @kidroca 👍

As I responded to your comment, I do think the refactor (moving ImageWithHeaders's loading logic into ImageLoader) could make this even more understandable, as long as that eliminates the need for the ImageWithHeaders component. Let me know what you think about my other comments below 👍

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

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

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

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

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

Beamanator · 2023-01-18T12:50:28Z

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

+    let uri;
+    const abortCtrl = new AbortController();
+    const request = new Request(nextSource.uri, {
+      headers: nextSource.headers,
+      signal: abortCtrl.signal
+    });
+    request.headers.append('accept', 'image/*');
+
+    if (onLoadStart) onLoadStart();
+
+    fetch(request)
+      .then((response) => response.blob())
+      .then((blob) => {
+        uri = URL.createObjectURL(blob);
+        setBlobUri(uri);
+      })
+      .catch((error) => {
+        if (error.name !== 'AbortError' && onError) {
+          onError({ nativeEvent: error.message });
+        }
+      });


Hmm since this logic is the real meat & potatoes of the ImageWithHeaders component, if we move it to ImageLoader.loadUsingHeaders I assume we could get rid of this ImageWithHeaders component, and basically check if there's headers passed in the props - if yes, we call ImageLoader.loadUsingHeaders instead of ImageLoader.load here?

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

Lines 269 to 294 in bd1f7b8

requestRef.current = ImageLoader.load(

uri,

function load(e) {

updateState(LOADED);

if (onLoad) {

onLoad(e);

}

if (onLoadEnd) {

onLoadEnd();

}

},

function error() {

updateState(ERRORED);

if (onError) {

onError({

nativeEvent: {

error: `Failed to load resource ${uri} (404)`

}

});

}

if (onLoadEnd) {

onLoadEnd();

}

}

);

}

If that's what you're thinking, I honestly do think that logic is cleaner, even without needing to write tests for Expensify's case so I'd say the refactor sounds like a nice idea

Move header loading logic here

kidroca

Moved logic to ImageLoader.loadUsingHeaders

✅ PR ready for review

And here's an alternative version where we try to use a single Image component instead of creating BaseImage and ImageWithHeaders: https://github.com/Expensify/react-native-web/pull/13/files

cc @roryabraham

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

kidroca · 2023-01-24T08:52:45Z

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

+  const request = React.useRef<LoadRequest>({
+    cancel: () => {},
+    source: { uri: '', headers: {} },
+    promise: Promise.resolve('')
+  });


ImageLoader.loadUsingHeaders returns a request with reference to the last loaded source, and a cleanup function. We no longer need to capture lastLoadedSource and cleanup refs

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

Beamanator

@kidroca thanks for much for keeping this PR around while also showing what #13 would look like - to be honest I am now thinking the ImageLoader changes look much simpler in this PR, and even I like how the logic for the ImageWithHeaders is separated out, so I'm actually go back on my previous thought, and I'd say this is good to merge 👍

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

Beamanator · 2023-01-24T15:11:18Z

@marcaaron I'll leave this unmerged for today in case you want to check out this one vs #13 - if you don't have any concerns (or if you're too busy today) I'm pretty confident we can move forward here so I'll merge tomorrow

marcaaron · 2023-01-24T17:20:15Z

Sounds good - I will take a look now!

Also, just wanted to clear the air on this comment. Chatted 1:1 with Alex and it's clear we had different expectations about how this PR should be led and who should do the bulk of the reviewing.

In retrospect, I should have clarified that stuff before leaving that comment. And it was wrong to imply that there were any bad intentions (definitely did not mean for it to come across that way). It also didn't need to be shared in public. Sorry Alex!

marcaaron

Code looks great. Love this new direction. I only have a few style notes and requests to improve the comments in the code.

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

marcaaron · 2023-01-24T17:38:05Z

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

+
+  const propsToPass = {
+    ...props,
+    // Omit `onLoadStart` because we trigger it in the current scope


Not too sure what this comment means. Is there a different way to say this?

I think it's something like - the BaseImage onLoadStart event is not exposed to the parent. We are only interested in when the source with headers starts loading and not when the BaseImage loading starts?

I think it's more like: ImageWithHeaders already calls onLoadStart when it starts loading the image, so we don't want the BaseImage to trigger that function a second time

Now that I look at it I see it's confusing - it's like Alex said - loading starts inside ImageWithHeaders, to prevent BaseImage to raise onLoadStart a second time we filter it out

Ok cool I think we are all saying the same thing - BaseImage will not use an onLoadStart callback when we have headers.

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

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

kidroca · 2023-01-25T10:01:39Z

I'll apply the suggestions to the PR and post back in a minute

Co-authored-by: Marc Glasser <[email protected]>

kidroca · 2023-01-25T10:46:37Z

@Beamanator @marcaaron
I've applied the suggested changes and updated / added code comments

I've also updated the PR in App Expensify/App#13036 to use the latest changes made here
I think it's looking good

Beamanator

Two more tiny questions on comments

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

Beamanator · 2023-01-25T12:38:20Z

Oh and one more question / possible point of confusion:

ImageWithHeaders loads an image with headers....

While it's loading, it passes source: undefined to BaseImage (since the blobUri hasn't been set yet and so BaseImage doesn't try to load any image yet)
After it loads, it passes source: { nextSource: { headers: {...} }, uri: blobUri} to BaseImage
- From here, BaseImage treats blobUri as a normal uri and loads it like a normal image (via ImageLoader.load), firing onError and / or onLoadEnd if that errors out, right?
- So we need to handle errors / loading ending in both BaseImage and in ImageWithHeaders because ImageWithHeaders will technically have 2 types of loading stages, each of which "can" error out and then need to call onLoadEnd?

Just making sure my understanding is correct 😅

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

Co-authored-by: Alex Beaman <[email protected]>

kidroca · 2023-01-26T14:50:16Z

@Beamanator

Oh and one more question / possible point of confusion:

ImageWithHeaders loads an image with headers....

While it's loading, it passes source: undefined to BaseImage (since the blobUri hasn't been set yet and so BaseImage doesn't try to load any image yet)

After it loads, it passes source: { nextSource: { headers: {...} }, uri: blobUri} to BaseImage

From here, BaseImage treats blobUri as a normal uri and loads it like a normal image (via ImageLoader.load), firing onError and / or onLoadEnd if that errors out, right?

So we need to handle errors / loading ending in both BaseImage and in ImageWithHeaders because ImageWithHeaders will technically have 2 types of loading stages, each of which "can" error out and then need to call onLoadEnd?

Just making sure my understanding is correct 😅

I think you've got it right

When we load image with headers the fetch request might fail - we call onError and onLoadEnd there
Otherwise (if fetch succeeds) onLoadEnd would be called from BaseImage
BaseImage might still raise onError if the fetched file is not an image

We only need to take care of the Promise rejection in ImageWithHeaders and raise error / loadEnd, BaseImage would take care of the rest like it already does

Beamanator

Thanks for all the work on this @kidroca 👍 👍

Beamanator · 2023-01-27T15:20:00Z

Published in v0.18.11 👍

roryabraham · 2023-01-30T18:21:14Z

@Beamanator @kidroca can you please include a link to the upstream PR in the description to make it easier to track this change in the upstream?

Beamanator · 2023-01-31T09:45:03Z

@roryabraham sure thing - the upstream PR is: necolas#2442 (I added to the OP too!)

kidroca mentioned this pull request Jan 6, 2023

Image: support ImageSource with headers #8

Closed

kidroca added 2 commits January 9, 2023 20:58

[add] Image source headers handling

059ab04

[add] Image: Append source to nativeEvent

4e6daca

kidroca force-pushed the image-loader-headers-alt branch from cf3bc09 to 4e6daca Compare January 9, 2023 18:58

[change] Image code comments

8f4d952

kidroca marked this pull request as ready for review January 15, 2023 21:12

kidroca commented Jan 17, 2023

View reviewed changes

kidroca mentioned this pull request Jan 17, 2023

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

Merged

56 tasks

Beamanator mentioned this pull request Jan 18, 2023

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

Closed

39 tasks

Beamanator reviewed Jan 18, 2023

View reviewed changes

[add] ImageLoader.loadUsingHeaders

1e54c64

Move header loading logic here

kidroca force-pushed the image-loader-headers-alt branch 2 times, most recently from 51f7e9a to 739c02b Compare January 24, 2023 08:40

Cleanup

93df02a

kidroca force-pushed the image-loader-headers-alt branch from 739c02b to 93df02a Compare January 24, 2023 08:50

kidroca commented Jan 24, 2023

View reviewed changes

Beamanator previously approved these changes Jan 24, 2023

View reviewed changes

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

marcaaron requested changes Jan 24, 2023

View reviewed changes

kidroca dismissed Beamanator’s stale review via 7e2d106 January 25, 2023 10:12

kidroca and others added 2 commits January 25, 2023 12:15

Apply suggestions from code review

7353183

Co-authored-by: Marc Glasser <[email protected]>

Image - clean up comments

bb25c15

kidroca force-pushed the image-loader-headers-alt branch from 7e2d106 to bb25c15 Compare January 25, 2023 10:31

kidroca mentioned this pull request Jan 25, 2023

[add] Image source headers handling (Single Component) #13

Closed

Beamanator requested changes Jan 25, 2023

View reviewed changes

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

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

marcaaron previously approved these changes Jan 25, 2023

View reviewed changes

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

Apply suggestions from code review

1f393c4

Co-authored-by: Alex Beaman <[email protected]>

kidroca dismissed marcaaron’s stale review via 1f393c4 January 26, 2023 14:45

Beamanator approved these changes Jan 27, 2023

View reviewed changes

Beamanator merged commit f8e6824 into Expensify:master Jan 27, 2023

roryabraham mentioned this pull request Jan 30, 2023

Undo fork from #11463 Expensify/App#13846

Closed

Beamanator mentioned this pull request Jan 31, 2023

[HOLD for payment 2024-03-29] [HOLD for payment 2024-03-26] [$250] [Feature] RNW Image: Add support for http properties in source Expensify/App#12603

Closed

kidroca mentioned this pull request Mar 9, 2023

[HOLD for payment 2023-04-03] Clean-up react-native-web workaround and fix it upstream Expensify/App#14278

Closed

kidroca deleted the image-loader-headers-alt branch April 7, 2023 07:26

roryabraham mentioned this pull request Aug 11, 2023

[$3500] Upgrade to react-native-web v0.19.6 or greater Expensify/App#16660

Closed

	requestRef.current = ImageLoader.load(
	uri,
	function load(e) {
	updateState(LOADED);
	if (onLoad) {
	onLoad(e);
	}
	if (onLoadEnd) {
	onLoadEnd();
	}
	},
	function error() {
	updateState(ERRORED);
	if (onError) {
	onError({
	nativeEvent: {
	error: `Failed to load resource ${uri} (404)`
	}
	});
	}
	if (onLoadEnd) {
	onLoadEnd();
	}
	}
	);
	}

[add] Image source headers handling #11

[add] Image source headers handling #11

Uh oh!

Conversation

kidroca commented Jan 6, 2023 • edited by Beamanator Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Details

Fixed Issues

Test Strategy

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca Jan 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca commented Jan 17, 2023

Uh oh!

Beamanator commented Jan 17, 2023

Uh oh!

marcaaron commented Jan 17, 2023

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kidroca left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Beamanator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Beamanator commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcaaron commented Jan 24, 2023

Uh oh!

marcaaron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kidroca commented Jan 6, 2023 •

edited by Beamanator

Loading

kidroca Jan 19, 2023 •

edited

Loading

kidroca Jan 20, 2023 •

edited

Loading

Beamanator commented Jan 24, 2023 •

edited

Loading

kidroca commented Jan 26, 2023 •

edited

Loading