Image: support ImageSource with headers #8

kidroca · 2022-11-17T16:11:55Z

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 the ImageSource
is just an uri with no headers

When headers are involved we use the fetch API to load the image

Why are there 2 ways to load images?
Because fetch or xhr does not work for

loading progressive JPEG images
loading crossorigin blobs

Cross origin image requests can still be made with headers (fetch) if the server is configured correctly

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

I think this is close to the final version of what we're looking for as changes in order to support Image sources with headers

I've tested locally on the dev server and the example content

Tomorrow I'll link and try the code in App

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

kidroca · 2022-11-17T17:05:08Z

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

-      if (typeof image.decode === 'function') {
-        // Safari currently throws exceptions when decoding svgs.
-        // We want to catch that error and allow the load handler
-        // to be forwarded to the onLoad handler in this case
-        image.decode().then(onDecode, onDecode);
-      } else {
-        setTimeout(onDecode, 0);
-      }


Removed the if (typeof image.decode === 'function') check and the setTimeout fallback, because the minimal supported browser versions are all capable of image.decode

I think we're probably OK to also remove this comment and handling about Safari since it's from 2018 and iOS 11
Looks like the bug was addressed for newer versions of Safari: https://bugs.webkit.org/show_bug.cgi?id=201243

I think since we don't need to remove this we should just leave it as it is and let the maintainer decide.

kidroca · 2022-11-17T17:07:03Z

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

+          image.src = URL.createObjectURL(blob);
+        })
+        .catch((error) => {
+          if (error.name !== 'AbortError') onError(error);


Skip raising onError for aborted requests

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

packages/react-native-web/src/exports/Image/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

marcaaron

Just left a few quick questions for now. I'd need to do a more thorough review of the existing code before diving into the material changes - but wondering if there is any simplifying we can do at a high level.

packages/react-native-web-examples/pages/image/index.js

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

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

kidroca

Hey guys, I'll need to modify this PR again because I've found some cases that I failed to cover

The things I've answered here are still valid though

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

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

kidroca · 2022-11-25T14:13:30Z

This is Ready for Review ✔️
I won't make anymore changes until a review

To test loading Images with Headers in App you can use the following PR which uses the bundled code: Expensify/App#13036

I've opened a PR for the mainstream repo as well and left notes on the changes: necolas#2442 (review)

Beamanator · 2022-12-02T11:51:34Z

Current holding on this PR till we see if upstream PR gets merged: necolas#2442

- extend ImageLoader.load params - Removed the old `image.decode` change as it's covered by the minimal browser versions supported here - add examples for Images with headers - Image - remove requestRef - no longer needed - rename `ImageLoader.abort` to `.release` The method is mostly used as cleanup (e.g. useEffect cleanup, or releasing resources when component unmounts) - Image - extract `useSource` hook Move the image loading effect here Changed the original logic slightly for less nesting Changed to cover cases where passing the same headers object was starting new loads, as it was treated as a different value due to referential equality - Image - add tests covering added/changed functionality around source - Image - handle cases where the source object only changes by reference When the source object changed by reference, but stays structurally the same, we should do nothing and not trigger the loading effect again - Image - extract ImageLoadingProps Update types to match RN and actual code - we don't call `onLoadStart` and `onLoadEnd` with any arguments - ImageLoader extract types.js - Image - resolve `onLoad` with `source` Use the same `nativeEvent` structure as in RN for the onLoad event - Rework Image loading and source management logic Since introducing the change to support headers changes to the original changes are needed: - support loading a default source with headers - handle source object changes - update uri resolving logic to handle blob URLs create by `URL.createObjectURL` - move the URI/source resolving logic to the `ImageLoader` BREAKING CHANGE `onLoad` was previously called with `nativeEvent` that was the browser Event object from the image.onload handler Since we can't spread or mutate the Event object to add `source` we have to either add it under a new key or remove it The browser Event does not expose very useful information, (no target, or size info), so it seems best to replace `nativeEvent` with the same structure used in `react-native`

Previously loaded images used to be added to ImageUriCache It seems the logic was accidentally removed here: necolas@f4e8b6b#diff-7cb74a3a32d73857be80350ecd1ea131d256bd5af11d2000e4fc2d03c2230584L361 And now the `ImageUriCache` is only updated by preload/getSize

marcaaron · 2022-12-29T19:06:17Z

@Beamanator Just checking in on this one - but are we still HOLD'ing?

Beamanator · 2023-01-02T15:36:26Z

@marcaaron no we're no longer holding on this one - I just got back from OOO sorry for the delay

I'm planning to review this (can use https://github.com/Expensify/App/pull/13036/files to help) - & I could use your help if you have time @marcaaron - then we can publish it and test in a App PR, while we're waiting for the upstream PR to be merged

Beamanator

A few comment changes and some questions about how a few things work.

@marcaaron do you think you & I can give a good enough review on this or do we need some help? 😅 (I'm thinking we could request a few more eyes)

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

packages/react-native-web-examples/pages/image/index.js

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

marcaaron · 2023-01-02T19:09:13Z

@marcaaron do you think you & I can give a good enough review on this or do we need some help? 😅 (I'm thinking we could request a few more eyes)

I would ask @tgolen to take a look as well. I am a short on context about the change and lacking familiarity with the library code (react-native-web) we are changing, but happy to take some time to review the code again.

tgolen

I feel pretty inadequate reviewing this code. A lot of the changes are unfamiliar to me, I am not sure why they are necessary, and I am not familiar with the syntax used.

I spotted a couple of things, but feel free to take this review with a grain of salt.

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

marcaaron

Left a few more thoughts. Sorry, I was only able to get about halfway.

I need to point out an "elephant in the room" which is that.. I honestly don't have a good understanding of the existing code. And I don't feel like I can give a proper review in it's current state (or at least not without some serious struggling).

I will leave the same feedback as before. It feels like we are making more changes than we need to.

Here's my suggestion...

If there is a refactor that can be avoided for now let's avoid it. Several times when reviewing this I had to stop and figure out what was new and what wasn't and what is a necessary refactor and whether we are just moving things around as code cleanup.

I think our goal at this point should be to get some mutual understanding of what changes we are making and why we are making them.

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

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

kidroca

Explaining some comments

I'll address whatever changes I can today

If there are any Flow issues I'll point out that the changes were necessary to satisfy the Flow checker

I'll also try to summarize the changes after I'm done

packages/react-native-web-examples/pages/image/index.js

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

Co-authored-by: Alex Beaman <[email protected]> Co-authored-by: Marc Glasser <[email protected]> Co-authored-by: Tim Golen <[email protected]>

marcaaron · 2023-01-04T16:41:17Z

If we need to get around flow can we use $FlowFixMe?

https://flow.org/en/docs/errors/

Just thinking if it was between having to refactor a bunch of stuff and just suppress whatever it is that is giving you issues I'd be fine with suppressing until this is ready to be merged upstream.

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

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

tgolen · 2023-01-05T00:30:21Z

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

  } else if (source && typeof source.uri === 'string') {
-    uri = source.uri;
+    const { uri, width, height, headers } = source;
+    resolvedSource = { uri, width, height, headers };


This kind of goes with the previous comment, but what properties are you trying to strip and why can't they remain?

The source object at this point resolves to the following shape

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

Lines 23 to 78 in bd1f7b8

type SourceObject = {

/**

* `body` is the HTTP body to send with the request. This must be a valid

* UTF-8 string, and will be sent exactly as specified, with no

* additional encoding (e.g. URL-escaping or base64) applied.

*/

body?: string,

/**

* `cache` determines how the requests handles potentially cached

* responses.

*

* - `default`: Use the native platforms default strategy. `useProtocolCachePolicy` on iOS.

*

* - `reload`: The data for the URL will be loaded from the originating source.

* No existing cache data should be used to satisfy a URL load request.

*

* - `force-cache`: The existing cached data will be used to satisfy the request,

* regardless of its age or expiration date. If there is no existing data in the cache

* corresponding the request, the data is loaded from the originating source.

*

* - `only-if-cached`: The existing cache data will be used to satisfy a request, regardless of

* its age or expiration date. If there is no existing data in the cache corresponding

* to a URL load request, no attempt is made to load the data from the originating source,

* and the load is considered to have failed.

*

* @platform ios

*/

cache?: 'default' | 'reload' | 'force-cache' | 'only-if-cached',

/**

* `headers` is an object representing the HTTP headers to send along with the

* request for a remote image.

*/

headers?: { [key: string]: string },

/**

* `method` is the HTTP Method to use. Defaults to GET if not specified.

*/

method?: string,

/**

* `scale` is used to indicate the scale factor of the image. Defaults to 1.0 if

* unspecified, meaning that one image pixel equates to one display point / DIP.

*/

scale?: number,

/**

* `uri` is a string representing the resource identifier for the image, which

* could be an http address, a local file path, or the name of a static image

* resource (which should be wrapped in the `require('./path/to/image.png')`

* function).

*/

uri: string,

/**

* `width` and `height` can be specified if known at build time, in which case

* these will be used to set the default `<Image/>` component dimensions.

*/

height?: number,

width?: number

};

This is a non-exact type, which means it can carry additional fields

We want to make sure we resolve a resolvedSource complying to the ImageSource exact type

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

Lines 12 to 17 in 6a5d237

export type ImageSource = {|

uri: string,

headers?: { [key: string]: string },

width?: ?number,

height?: ?number

|};

And so we destruct only the necessary fields

The reason we want to comply with a ImageSource type is to make the rest of the code simpler

E.g. source can change but this should not trigger image loading unless any of the image loading fields changed
We achieve this by returning the same resolved source, because any arbitrary fields would be stripped

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

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

tgolen · 2023-01-05T00:38:30Z

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

+
+    // It's not possible to use headers with `image.src`, that's why we use a different strategy for sources with headers
+    if (source.headers) {
+      const abortCtrl = new AbortController();


Maybe a code comment would help here, but is the reason for the abort controller so that if the component unmounts before the image is loaded, then it cleans up a memory leak?

Yes and more

If the Image component starts a new request, the old one is discarded - we want to stop fetching it, and focus on the new request
Same thing if the component unmounts, we want to stop loading

There's an ImageLoader.abort method, I think any description about why it exists and when to use it should be there
The specific code here is just so the ImageLoader.abort method can stop the newly added code working with fetch

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

kidroca

Reverted as much of the refactor changes, bug fixes and optimizations as I can

If you see anything unnecessary do point it out

Otherwise this is ready for review ✔️

kidroca · 2023-01-05T11:23:20Z

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

+    if (asset == null) {
+      throw new Error(
+        `Image: asset with ID "${source}" could not be found. Please check the image source or packager.`
+      );
+    }


This change comes from the upstream repo, where I needed to include it to fix merge conflicts and make my PR mergeable

kidroca · 2023-01-05T11:26:45Z

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

-    uri = asset
-      ? `${asset.httpServerLocation}/${asset.name}${scaleSuffix}.${asset.type}`
-      : '';


This line were changed because of the added if (asset == null) check on L104

At this point asset cannot be null or undefined so the ternary was removed

kidroca · 2023-01-05T11:29:05Z

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

+    // $FlowFixMe
+    const { uri, width, height, headers } = source;


// $FlowFixMe added because Flow thinks the source might be an Array here and produces a few errors

kidroca · 2023-01-05T12:04:13Z

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

  } else if (source && typeof source.uri === 'string') {
-    uri = source.uri;
+    const { uri, width, height, headers } = source;
+    resolvedSource = { uri, width, height, headers };


The source object at this point resolves to the following shape

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

Lines 23 to 78 in bd1f7b8

type SourceObject = {

/**

* `body` is the HTTP body to send with the request. This must be a valid

* UTF-8 string, and will be sent exactly as specified, with no

* additional encoding (e.g. URL-escaping or base64) applied.

*/

body?: string,

/**

* `cache` determines how the requests handles potentially cached

* responses.

*

* - `default`: Use the native platforms default strategy. `useProtocolCachePolicy` on iOS.

*

* - `reload`: The data for the URL will be loaded from the originating source.

* No existing cache data should be used to satisfy a URL load request.

*

* - `force-cache`: The existing cached data will be used to satisfy the request,

* regardless of its age or expiration date. If there is no existing data in the cache

* corresponding the request, the data is loaded from the originating source.

*

* - `only-if-cached`: The existing cache data will be used to satisfy a request, regardless of

* its age or expiration date. If there is no existing data in the cache corresponding

* to a URL load request, no attempt is made to load the data from the originating source,

* and the load is considered to have failed.

*

* @platform ios

*/

cache?: 'default' | 'reload' | 'force-cache' | 'only-if-cached',

/**

* `headers` is an object representing the HTTP headers to send along with the

* request for a remote image.

*/

headers?: { [key: string]: string },

/**

* `method` is the HTTP Method to use. Defaults to GET if not specified.

*/

method?: string,

/**

* `scale` is used to indicate the scale factor of the image. Defaults to 1.0 if

* unspecified, meaning that one image pixel equates to one display point / DIP.

*/

scale?: number,

/**

* `uri` is a string representing the resource identifier for the image, which

* could be an http address, a local file path, or the name of a static image

* resource (which should be wrapped in the `require('./path/to/image.png')`

* function).

*/

uri: string,

/**

* `width` and `height` can be specified if known at build time, in which case

* these will be used to set the default `<Image/>` component dimensions.

*/

height?: number,

width?: number

};

This is a non-exact type, which means it can carry additional fields

We want to make sure we resolve a resolvedSource complying to the ImageSource exact type

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

Lines 12 to 17 in 6a5d237

export type ImageSource = {|

uri: string,

headers?: { [key: string]: string },

width?: ?number,

height?: ?number

|};

And so we destruct only the necessary fields

The reason we want to comply with a ImageSource type is to make the rest of the code simpler

E.g. source can change but this should not trigger image loading unless any of the image loading fields changed
We achieve this by returning the same resolved source, because any arbitrary fields would be stripped

kidroca · 2023-01-05T12:19:02Z

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

-  const displayImageUri = resolveAssetUri(selectedSource);
-  const imageSizeStyle = resolveAssetDimensions(selectedSource);


resolveAssetUri was changed to resolveSource and it takes care or resolving the URI and the dimensions

kidroca · 2023-01-05T12:42:29Z

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

+        onLoad({
+          source: {
+            uri: image.src,
+            width: image.naturalWidth,
+            height: image.naturalHeight
+          }
+        });


Here's what onLoad was previously called with

Since target, currentTarget and srcElement are all null I see nothing useful for anybody to be using the data coming from the current onLoad implementation

There's a way to get dimensions by traversing e.path[0].naturalWidth

kidroca · 2023-01-05T12:52:22Z

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

+
+    // It's not possible to use headers with `image.src`, that's why we use a different strategy for sources with headers
+    if (source.headers) {
+      const abortCtrl = new AbortController();


Yes and more

If the Image component starts a new request, the old one is discarded - we want to stop fetching it, and focus on the new request
Same thing if the component unmounts, we want to stop loading

There's an ImageLoader.abort method, I think any description about why it exists and when to use it should be there
The specific code here is just so the ImageLoader.abort method can stop the newly added code working with fetch

marcaaron

Thanks for simplifying @kidroca. Much easier to understand the changes here. I left a few more questions and comments.

marcaaron · 2023-01-05T22:35:27Z

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

+      // Since `defaultSource` is meant to be displayed until `source` loads
+      // we resolve it first (otherwise it won't be displayed at all)
+      act(() => {
+        const call = calls.find(({ source }) => source.uri === defaultUri);


Seems like the previous test had a snapshot before anything was resolved. Do we still need that?

Seems like the previous test had a snapshot before anything was resolved. Do we still need that?

The loading behavior changed and so the test changed

Previously the default source was not loaded using the ImageLoader
Now it is (so it can also use headers if necessary)
So we first need to resolve it by resolving the ImageLoader.load mock

marcaaron · 2023-01-05T23:21:10Z

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

-
+  // Only the main source is supposed to trigger onLoad/start/end events
+  // It would be ambiguous to trigger the same `onLoad` event when default source loads
+  // That's why we don't pass `onLoad` props for the fallback source hook


This comment is confusing to me.

It would be ambiguous to trigger the same onLoad event when default source loads

What does "ambiguous" mean in this context?

The ambiguous part is this

Let's have a use case where both source, defaultSource and onLoad are passed
If we trigger onLoad when defaultSource loads and then trigger onLoad again when source loads it would be confusing

why was onLoad triggered twice - Oh, ok because we use 2 sources

Ok but then how do I know which source triggered onLoad - which one loaded first

The original behavior is to only trigger load events for the source
Previously defaultSource was not loaded using ImageLoader.load and it wasn't even possible to trigger load events for it

Now since both sources reuse the useSource hook, I try to explain why one has onLoad and the other does not

Ah ok, I see now that only the "main" source can trigger an onLoad event.

Is it at all useful to know when the defaultSource loads?

If not, I'm not sure we need to explain why we are not doing something. Though, if it's useful and the problem is "ambiguity" then we could solve it with a more specific callback like onDefaultSourceLoad.

marcaaron · 2023-01-05T23:28:35Z

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

+/**
+ * Image loading/state management hook
+ */
+const useSource = (callbacks, source: ?Source) => {


Should we add a doc or type for callbacks?

I did add a type originally, but removed it here to reduce the stuff I have to explain

9edf19b#diff-5a9c8a4b5a6a7db53c378d2191c7902c66a602d85a8991db018a80eacebb0239L106-L121

I can easily add it back - do let me know

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

marcaaron · 2023-01-06T00:37:37Z

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

      image.onerror = null;
      image.onload = null;
-      image = null;
+      // Setting image.src to empty string aborts any ongoing image loading


Why does setting this to an empty string abort the load? Is this a standard behavior on all browsers at this point?

Why does setting this to an empty string abort the load? Is this a standard behavior on all browsers at this point?

AFAIK it's mostly a vendor courtesy so it's not a standard behavior: https://stackoverflow.com/a/5278475/4834103
But I think the most popular browsers work that way

Cool. Yeah I had some trouble finding details on this but ran into a pretty old article that suggested it would break the entire internet 😂

marcaaron · 2023-01-06T00:45:13Z

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

      // avoid blocking the main thread
-      const onDecode = () => onLoad({ nativeEvent: e });


Looks like this would break any web platform code where onLoad events are expecting the nativeEvent?

Looks like this would break any web platform code where onLoad events are expecting the nativeEvent?

This was moved to the Image component
We still call onLoad with an object containing nativeEvent here:

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

Line 345 in 9edf19b

if (onLoad) onLoad({ nativeEvent: result });

though the structure did change (noted in other comment)

I've updated the code and avoided this breaking change
I guess I just missed to see that source was being successfully added on the event object

Now it would be up to the maintainer to decide whether they want to leave just source or keep the rest of the data

marcaaron · 2023-01-06T00:57:27Z

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

+        onLoad({
+          source: {
+            uri: image.src,
+            width: image.naturalWidth,
+            height: image.naturalHeight
+          }
+        });


Made this match the RN onLoad interface

It looks like that link shared and the event still has the nativeEvent object in it (maybe I am reading it wrong). Seems like we do not need to remove it? If we can do what we need to do without a breaking change that would be good. Seems better left to the maintainers than us.

marcaaron · 2023-01-06T00:58:19Z

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

-      if (typeof image.decode === 'function') {
-        // Safari currently throws exceptions when decoding svgs.
-        // We want to catch that error and allow the load handler
-        // to be forwarded to the onLoad handler in this case
-        image.decode().then(onDecode, onDecode);
-      } else {
-        setTimeout(onDecode, 0);
-      }


I think since we don't need to remove this we should just leave it as it is and let the maintainer decide.

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

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

Extract callbacks from ref before usage - not when loading starts

kidroca · 2023-01-06T22:05:45Z

@marcaaron @tgolen @Beamanator
I came up with an alternative approach with less changes
It's like a HOC component that resolves local blob url and delegates it to the original image component

#11

I can't decide which PR is better, I guess it would be the new one as it's less code to own/maintain by Expensify

We append `source` to the `nativeEvent` raised by the Image.onload event in order to preserve existing functionality but mach the `source` object available in react-native's image load event

marcaaron · 2023-01-09T18:53:10Z

Ah well, I think we are getting closer here, but the new PR does look better to me. 😄

kidroca · 2023-01-09T20:15:38Z

Ah well, I think we are getting closer here, but the new PR does look better to me. 😄

The only down sides I can think are

default source is not going to be loaded using headers
unit test have to be a bit different (but also less)

Regarding the default source issue we can do a follow up PR where
defaultSource is extracted out of the BaseImage and we have a higher component that decides to render source or defaultSource, but then it uses the same component to render the selected source

kidroca · 2023-01-13T10:03:51Z

@marcaaron @tgolen @Beamanator
What should I do next?

Everything on the current PR was addressed and it's ready for review
I've proposed an alternative PR with less changes, but no headers for defaultSource

Do you want me to continue on 2) and add some unit tests there?
Otherwise I've tested 2) in App and it also covers our use cases (It's ready for review as well)

(I'm not sure whether someone would need to use defaultSource with headers as it's meant to be a placeholder/fallback while actual content is loading, but if there's need it ca be addressed as a follow up PR)

Beamanator · 2023-01-13T16:52:35Z

@kidroca As far as I can see, we don't currently use the defaultSource prop when trying to display chat attachment images, so I don't see that being a problem, and seeing that #11 is less than 1/3 of the changes in this PR, I feel like it makes sense to go that route.

@marcaaron what do you think? You've put a decent amount of effort into reviewing this PR so if you're confident with it at this point (seeing that your latest comments were addressed) maybe we can move forward with this one - otherwise #11 is looking nice and slim and delicious

marcaaron · 2023-01-13T16:56:54Z

Thanks, I agree. let's close this one and move the conversation over to the new PR.

kidroca · 2023-01-15T21:12:38Z

Closing this PR in favor of #11

kidroca commented Nov 17, 2022

View reviewed changes

kidroca marked this pull request as ready for review November 18, 2022 19:53

kidroca mentioned this pull request Nov 19, 2022

[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

marcaaron reviewed Nov 21, 2022

View reviewed changes

Beamanator self-requested a review November 23, 2022 14:07

kidroca commented Nov 24, 2022

View reviewed changes

kidroca force-pushed the kidroca/feat/image-loader-headers branch 7 times, most recently from ddbcae4 to e5a4597 Compare November 25, 2022 13:24

Beamanator mentioned this pull request Dec 20, 2022

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

Closed

39 tasks

kidroca added 2 commits December 21, 2022 13:53

kidroca force-pushed the kidroca/feat/image-loader-headers branch from e5a4597 to e2248d6 Compare December 21, 2022 11:59

Beamanator requested changes Jan 2, 2023

View reviewed changes

tgolen previously approved these changes Jan 2, 2023

View reviewed changes

marcaaron requested changes Jan 2, 2023

View reviewed changes

kidroca commented Jan 4, 2023

View reviewed changes

Apply suggested typo fixes and clarifications

6429c1e

Co-authored-by: Alex Beaman <[email protected]> Co-authored-by: Marc Glasser <[email protected]> Co-authored-by: Tim Golen <[email protected]>

kidroca dismissed tgolen’s stale review via 6429c1e January 4, 2023 15:11

Remove resolveSource helper functions

9ef7d28

tgolen requested changes Jan 5, 2023

View reviewed changes

kidroca force-pushed the kidroca/feat/image-loader-headers branch 2 times, most recently from 6a5d237 to 38634e0 Compare January 5, 2023 12:16

Reducing changes

9edf19b

kidroca force-pushed the kidroca/feat/image-loader-headers branch from 38634e0 to 9edf19b Compare January 5, 2023 12:32

kidroca commented Jan 5, 2023

View reviewed changes

kidroca requested review from tgolen, marcaaron and Beamanator and removed request for tgolen, marcaaron and Beamanator January 5, 2023 12:59

marcaaron requested changes Jan 6, 2023

View reviewed changes

kidroca and others added 3 commits January 6, 2023 15:23

Clarify code comments

4227410

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

Add back the image.decode compatibility check

10df92e

Fix callbacks ref

9d6cbe7

Extract callbacks from ref before usage - not when loading starts

Alter onLoad breaking change

42ef468

We append `source` to the `nativeEvent` raised by the Image.onload event in order to preserve existing functionality but mach the `source` object available in react-native's image load event

kidroca closed this Jan 15, 2023

	type SourceObject = {
	/**
	* `body` is the HTTP body to send with the request. This must be a valid
	* UTF-8 string, and will be sent exactly as specified, with no
	* additional encoding (e.g. URL-escaping or base64) applied.
	*/
	body?: string,
	/**
	* `cache` determines how the requests handles potentially cached
	* responses.
	*
	* - `default`: Use the native platforms default strategy. `useProtocolCachePolicy` on iOS.
	*
	* - `reload`: The data for the URL will be loaded from the originating source.
	* No existing cache data should be used to satisfy a URL load request.
	*
	* - `force-cache`: The existing cached data will be used to satisfy the request,
	* regardless of its age or expiration date. If there is no existing data in the cache
	* corresponding the request, the data is loaded from the originating source.
	*
	* - `only-if-cached`: The existing cache data will be used to satisfy a request, regardless of
	* its age or expiration date. If there is no existing data in the cache corresponding
	* to a URL load request, no attempt is made to load the data from the originating source,
	* and the load is considered to have failed.
	*
	* @platform ios
	*/
	cache?: 'default' \| 'reload' \| 'force-cache' \| 'only-if-cached',
	/**
	* `headers` is an object representing the HTTP headers to send along with the
	* request for a remote image.
	*/
	headers?: { [key: string]: string },
	/**
	* `method` is the HTTP Method to use. Defaults to GET if not specified.
	*/
	method?: string,
	/**
	* `scale` is used to indicate the scale factor of the image. Defaults to 1.0 if
	* unspecified, meaning that one image pixel equates to one display point / DIP.
	*/
	scale?: number,
	/**
	* `uri` is a string representing the resource identifier for the image, which
	* could be an http address, a local file path, or the name of a static image
	* resource (which should be wrapped in the `require('./path/to/image.png')`
	* function).
	*/
	uri: string,
	/**
	* `width` and `height` can be specified if known at build time, in which case
	* these will be used to set the default `<Image/>` component dimensions.
	*/
	height?: number,
	width?: number
	};

	export type ImageSource = {\|
	uri: string,
	headers?: { [key: string]: string },
	width?: ?number,
	height?: ?number
	\|};

		// $FlowFixMe
		const { uri, width, height, headers } = source;

		const displayImageUri = resolveAssetUri(selectedSource);
		const imageSizeStyle = resolveAssetDimensions(selectedSource);

		// avoid blocking the main thread
		const onDecode = () => onLoad({ nativeEvent: e });

Image: support ImageSource with headers #8

Image: support ImageSource with headers #8

Conversation

kidroca commented Nov 17, 2022

Details

Fixed Issues

Test Strategy

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

marcaaron left a comment

Choose a reason for hiding this comment

kidroca left a comment

Choose a reason for hiding this comment

kidroca commented Nov 25, 2022

Beamanator commented Dec 2, 2022

marcaaron commented Dec 29, 2022

Beamanator commented Jan 2, 2023

Beamanator left a comment

Choose a reason for hiding this comment

marcaaron commented Jan 2, 2023

tgolen left a comment

Choose a reason for hiding this comment

marcaaron left a comment

Choose a reason for hiding this comment

kidroca left a comment

Choose a reason for hiding this comment

marcaaron commented Jan 4, 2023

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

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

Choose a reason for hiding this comment

marcaaron 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidroca commented Jan 6, 2023 • edited Loading

marcaaron commented Jan 9, 2023

kidroca commented Jan 9, 2023 • edited Loading

kidroca commented Jan 13, 2023

Beamanator commented Jan 13, 2023

marcaaron commented Jan 13, 2023

kidroca commented Jan 15, 2023

kidroca commented Jan 6, 2023 •

edited

Loading

kidroca commented Jan 9, 2023 •

edited

Loading