-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
do not reload errored tiles #6813
Conversation
Does this affect #6768 at all? |
@ryanhamley no, this doesn't change the behavior of #6768 as far as I can tell |
src/source/source_cache.js
Outdated
@@ -215,7 +215,7 @@ class SourceCache extends Evented { | |||
this._cache.reset(); | |||
|
|||
for (const i in this._tiles) { | |||
this._reloadTile(i, 'reloading'); | |||
if (this._tiles[i] && this._tiles[i].state !== "errored") this._reloadTile(i, 'reloading'); |
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.
Is the first part of the condition necessary, since we're already iterating through the keys of this._tiles
?
|
||
t.ok(reloadTileSpy.notCalled); | ||
|
||
t.end(); |
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 had to dig in a bit to convince myself that test would cause reload()
to be executed. To make it more obvious, could we have loadTile
load one successful tile and one errored tile, and then make sure that we have only one call to _reloadTile
?
👍 🐑 |
fix #6783
errored tiles were being reloaded, and their state was being changed from
errored
->reloading
which results intile.hasData()
to returntrue
, preventing the existing parent tiles from being retained correctly.Launch Checklist