Conversation
|
Thanks for the PR! The unit tests are currently failing but at a glance this direction looks good to me, and should allow onProgress to be implemented too. So I'll work with you to get this merged |
|
@necolas great! 😊 I just fixed some minor issues and now all tests are running fine. Also, I added the |
|
|
||
| static has(uri: string) { | ||
| static createCacheId(source: ImageSource) { | ||
| return JSON.stringify(ImageLoader.resolveSource(source)); |
There was a problem hiding this comment.
I'm a little concerned about using JSON.stringify here. We could use a Map instead
There was a problem hiding this comment.
Yeah, I don't like it either but I could not find a smart way. The advantage of this approach is that you will always get the cached content for a combination of method, uri, headers and body. If we use a Map instead and use the source as a key then we somehow first need to get the reference to the source which leads to the same issue again. Or did you have something else in mind?
There was a problem hiding this comment.
Yes, it allows object as keys but the reference is stored as a key and not the content of the object:
const map = new Map()
const key = { a: 1 }
map.set(key, true)
map.get(key) !== map.get({ a: 1 }) // trueThere was a problem hiding this comment.
Yeah that's a problem given how image sources are defined. I guess we're stuck with stringifying for now
| uri = source; | ||
| } else if (source && typeof source.uri === 'string') { | ||
| uri = source.uri; | ||
| const getCacheUrl = e => { |
There was a problem hiding this comment.
This looks a bit fragile. Why do we need to get the url from the native event?
There was a problem hiding this comment.
If an image is loaded via XHR, the actual image src will be replaced by a blob:
Since the onload event is triggered by the image itself, we can retrieve the new blob source in Image via the image src in the event handler without passing additional parameters. Though, of course we could also make this more explicit and pass the event and the new blob url as separate parameters in onLoad. What do you prefer?
There was a problem hiding this comment.
Can we store the blob when we get it, so we don't have to read it back from the native event?
There was a problem hiding this comment.
Yep, the blob will be stored once the image was loaded:
ImageUriCache.add(source, getCacheUrl(e));Afterwards it shouldn't be triggered again as long as the source does not change. Or do you mean something else?
There was a problem hiding this comment.
I'm wondering why we need to get the blob from the event object when we know what it is before that
There was a problem hiding this comment.
I just reworked that part and now the blob is returned directly from the ImageLoader.
This comment has been minimized.
This comment has been minimized.
|
@necolas I am just back from vacation and saw that there are conflicting files. Though, I don't see any changes in the affected files and am not sure why it says there are conflicts. Any idea what's wrong here? |
|
The conflicts are because the docs were rewritten. And now there is also a7ab961 |
|
#2442 is a contemporary fix for this. Thanks |
3 participants
This PR introduces support for Image source objects (related issue: #1019) which allows you to pass different headers or fetch data via POST method.
In the example I added a dynamic chart from Google Charts. Not directly related but to fill the row I added WebP and the mentioned redirect case in the issue.
Please let me know if anything is missing.