-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Image: support ImageSource with headers #2442
base: master
Are you sure you want to change the base?
Changes from all commits
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,6 +8,7 @@ | |
* @flow | ||
*/ | ||
|
||
import type { ImageSource, LoadRequest } from '../../modules/ImageLoader'; | ||
import type { ImageProps } from './types'; | ||
|
||
import * as React from 'react'; | ||
|
@@ -165,6 +166,23 @@ function resolveAssetUri(source): ?string { | |
return uri; | ||
} | ||
|
||
function raiseOnErrorEvent(uri, { onError, onLoadEnd }) { | ||
if (onError) { | ||
onError({ | ||
nativeEvent: { | ||
error: `Failed to load resource ${uri} (404)` | ||
} | ||
}); | ||
} | ||
if (onLoadEnd) onLoadEnd(); | ||
} | ||
Comment on lines
+169
to
+178
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was extracted for reuse out of the original Image component hook |
||
|
||
function hasSourceDiff(a: ImageSource, b: ImageSource) { | ||
return ( | ||
a.uri !== b.uri || JSON.stringify(a.headers) !== JSON.stringify(b.headers) | ||
); | ||
} | ||
|
||
interface ImageStatics { | ||
getSize: ( | ||
uri: string, | ||
|
@@ -177,10 +195,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 { | ||
'aria-label': ariaLabel, | ||
blurRadius, | ||
|
@@ -300,16 +320,7 @@ const Image: React.AbstractComponent< | |
}, | ||
function error() { | ||
updateState(ERRORED); | ||
if (onError) { | ||
onError({ | ||
nativeEvent: { | ||
error: `Failed to load resource ${uri} (404)` | ||
} | ||
}); | ||
} | ||
if (onLoadEnd) { | ||
onLoadEnd(); | ||
} | ||
raiseOnErrorEvent(uri, { onError, onLoadEnd }); | ||
} | ||
); | ||
} | ||
|
@@ -353,14 +364,76 @@ const Image: React.AbstractComponent< | |
); | ||
}); | ||
|
||
Image.displayName = 'Image'; | ||
BaseImage.displayName = 'Image'; | ||
|
||
/** | ||
* 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; | ||
Comment on lines
+369
to
+375
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps there's a way to declare I guess I need to declare |
||
const [blobUri, setBlobUri] = React.useState(''); | ||
const request = React.useRef<LoadRequest>({ | ||
cancel: () => {}, | ||
source: { uri: '', headers: {} }, | ||
promise: Promise.resolve('') | ||
}); | ||
|
||
const { onError, onLoadStart, onLoadEnd } = props; | ||
|
||
React.useEffect(() => { | ||
if (!hasSourceDiff(nextSource, request.current.source)) { | ||
return; | ||
} | ||
|
||
// When source changes we want to clean up any old/running requests | ||
request.current.cancel(); | ||
|
||
if (onLoadStart) { | ||
onLoadStart(); | ||
} | ||
|
||
// Store a ref for the current load request so we know what's the last loaded source, | ||
// and so we can cancel it if a different source is passed through props | ||
request.current = ImageLoader.loadWithHeaders(nextSource); | ||
|
||
request.current.promise | ||
.then((uri) => setBlobUri(uri)) | ||
.catch(() => | ||
raiseOnErrorEvent(request.current.source.uri, { onError, onLoadEnd }) | ||
); | ||
}, [nextSource, onLoadStart, onError, onLoadEnd]); | ||
|
||
// Cancel any request on unmount | ||
React.useEffect(() => request.current.cancel, []); | ||
|
||
const propsToPass = { | ||
...props, | ||
|
||
// `onLoadStart` is called from the current component | ||
// We skip passing it down to prevent BaseImage raising it a 2nd time | ||
onLoadStart: undefined, | ||
|
||
// Until the current component resolves the request (using headers) | ||
// we skip forwarding the source so the base component doesn't attempt | ||
// to load the original source | ||
source: blobUri ? { ...nextSource, uri: blobUri } : undefined | ||
}; | ||
|
||
return <BaseImage ref={ref} {...propsToPass} />; | ||
}); | ||
|
||
// $FlowIgnore: This is the correct type, but casting makes it unhappy since the variables aren't defined yet | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When this old
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think I've seen the original error the comment references at some point |
||
const ImageWithStatics = (Image: React.AbstractComponent< | ||
ImageProps, | ||
React.ElementRef<typeof View> | ||
> & | ||
ImageStatics); | ||
const ImageWithStatics: ImageComponent & ImageStatics = React.forwardRef( | ||
(props, ref) => { | ||
if (props.source && props.source.headers) { | ||
return <ImageWithHeaders ref={ref} {...props} />; | ||
} | ||
|
||
return <BaseImage ref={ref} {...props} />; | ||
} | ||
); | ||
|
||
ImageWithStatics.getSize = function (uri, success, failure) { | ||
ImageLoader.getSize(uri, success, failure); | ||
|
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.
Fake timers no longer needed to pass this test
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.
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