Guard against missing currentGallery element #124
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When an image is loaded, it can preload the next/previous image with a callback. It's possible that between when the decision to preload is made and the `loadImage` call for preloading happens, the current gallery has been modified and the index to preload is missing, causing the preload callback to error.
|
Thanks for reporting that. I'm currently unavailable, so I'm gonna need more time to investigate it. |
feimosi
requested changes
Oct 23, 2016
| // but by the time the load happens, the element is gone. | ||
| if (typeof galleryItem === 'undefined') { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I'd rename the variable to galleryImage and move the whole to the top to become:
var imageContainer = imagesElements[index];
var galleryImage = currentGallery[index];
// Return if the index exceeds prepared images in the overlay
// or if the current gallery has been changed / closed
if (imageContainer === undefined || galleryImage === undefined) {
return;
}I've also updated the comment. Let me know what do you think.
|
Your reasoning makes perfect sense and that may be the case. See my review and thanks for the contribution! |
|
Ok, updated. |
|
Perfect, thanks! |
feimosi
approved these changes
Oct 25, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an image is loaded,
it can preload the next/previous image with a callback.
It's possible that between when the decision to preload is made
and the
loadImagecall for preloading happens,the current gallery has been modified and the index to preload
is missing, causing the preload callback to error.
I can't say exactly why this is occurring, but each day we get at least one error on this line, with a stacktrace coming from
preloadNextorpreloadPrev, and this change appears to fix it. I decided to not invoke the callback in the case the chosen item is missing from the current gallery, since that's what you do on line 474 (ifimagesElements[index]is undefined). But since I can't create a reliable repro, I could use your judgement here.