-
Notifications
You must be signed in to change notification settings - Fork 14
[add] Image source headers handling #11
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
Changes from 3 commits
059ab04
4e6daca
8f4d952
1e54c64
93df02a
7353183
bb25c15
1f393c4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -8,7 +8,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||
* @flow | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
import type { ImageProps } from './types'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
import type { ImageProps, SourceObject } from './types'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
import * as React from 'react'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
import createElement from '../createElement'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -146,6 +146,12 @@ function resolveAssetUri(source): ?string { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
return uri; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
function hasSourceDiff(a: SourceObject, b: SourceObject) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
return ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
a.uri !== b.uri || JSON.stringify(a.headers) !== JSON.stringify(b.headers) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
Beamanator marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
interface ImageStatics { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
getSize: ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
uri: string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -158,10 +164,12 @@ interface ImageStatics { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
) => Promise<{| [uri: string]: 'disk/memory' |}>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
const Image: React.AbstractComponent< | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
type ImageComponent = React.AbstractComponent< | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
ImageProps, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
React.ElementRef<typeof View> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
> = React.forwardRef((props, ref) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
>; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
const BaseImage: ImageComponent = React.forwardRef((props, ref) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
accessibilityLabel, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
blurRadius, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -332,24 +340,94 @@ const Image: React.AbstractComponent< | |||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Image.displayName = 'Image'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
* This component handles specifically loading an image source with header | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
kidroca marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const ImageWithHeaders: ImageComponent = React.forwardRef((props, ref) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
// $FlowIgnore | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const nextSource: SourceObject = props.source; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const prevSource = React.useRef<SourceObject>({}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const cleanup = React.useRef<Function>(() => {}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
const [blobUri, setBlobUri] = React.useState(''); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
const { onError, onLoadStart } = props; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
React.useEffect(() => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!hasSourceDiff(nextSource, prevSource.current)) return; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
// When source changes we want to clean up any old/running requests | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
cleanup.current(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
prevSource.current = nextSource; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
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(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
kidroca marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andloadWithHeaders
need to work in a similar way in order to be interchangeable (so they were modified to return arequest.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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 toImageLoader
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 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Beamanator marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok cool I think we are all saying the same thing - BaseImage
will not use an onLoadStart
callback when we have headers.
Uh oh!
There was an error while loading. Please reload this page.