-
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
Prevent 404 HTTP code from throwing an exception #4446
Conversation
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.
🎉 this will make users so happy! just a couple questions on our level of certainty that 404
is only returned in the case of a (correctly) missing tile and not for other legitimate errors
debug/index.html
Outdated
}, | ||
center: [-120.8, 39.1], // starting position | ||
zoom: 11 // starting zoom | ||
|
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.
do you want to add this permanently? I'd prefer to keep debug page simple but open to arguments 😊
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.
Oopsie. I meant to overwrite this before pushing.
@@ -165,7 +165,7 @@ class SourceCache extends Evented { | |||
_tileLoaded(tile, id, previousState, err) { | |||
if (err) { | |||
tile.state = 'errored'; | |||
this._source.fire('error', {tile: tile, error: err}); | |||
if (err.status !== 404) this._source.fire('error', {tile: tile, error: err}); |
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.
are there any other cases in which a tile error would 404 for a legitimate reason besides no data in the tile (and thus no tile) ? Should we check the message matches 'Error: not found' ?
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.
The 404 status code strongly implies the "Not found" status text.
I cannot imagine any cases in which a custom status message on a 404 response would necessitate different error handling. If we discover one in the future, we can accommodate it then. 😄
src/source/vector_tile_source.js
Outdated
@@ -87,7 +87,8 @@ class VectorTileSource extends Evented { | |||
return; | |||
|
|||
if (err) { | |||
return callback(err); | |||
if (err.status !== 404) callback(err); |
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.
but do we still need to call the callback (w/o err
) if the tile is not found?
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.
Yup. The latest version of this file didn't get pushed either. Fixed now.
super(message); | ||
this.status = status; | ||
} | ||
} |
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.
👍
Hi. I think there's a problem with completely preventing the expception. If you are waiting for the "idle" event and the 404 error prints on the browser, the idle event does not fire, so my program keeps hanging. Having the exception would allow my program to move on. Is there a workaround to this? |
This PR prevents throwing an exception when a tile request yields a 404 HTTP status code. It also minorly improves our test coverage of the
ajax.js
util module.Fixes #1800
Launch Checklist