Skip to content
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.

Sign up for GitHub

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

Jump to bottom

no-callback fix for jsonp with server error #1070

Merged
merged 2 commits into from
Mar 23, 2018
Merged

no-callback fix for jsonp with server error #1070

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46c6c34
no-callback fix for jsonp with server error
Mar 13, 2018
287ba48
jsonp unknown error message update
strajuser Mar 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/Request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,20 @@ export function jsonp (url, params, callback, context) {
script.type = 'text/javascript';
script.src = url + '?' + serialize(params);
script.id = callbackId;
script.onerror = function (error) {
if (error && window._EsriLeafletCallbacks[callbackId] !== true) {
// Can't get true error code: it can be 404, or 401, or 500
var err = {
error: {
code: 500,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't know what the actual error was, best not to assign a code arbitrarily.

what about something like this?

var err = {
  message: 'An unknown error occurred'
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, it makes sense. I've just tried to save error contract as

var err = {
  error: {
     code: 'need something undefined',
     message: 'An unknown error occurred'
  }
}

May be close this PR and make another with correct username and message fix?
And what about failed tests?

Copy link
Contributor

@jgravois jgravois Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just tried to save error contract

meh, this guy seems to agree with you, so i can live with a 500 if you update the message.

May be close this PR and make another with correct username

no need to close this PR. just push another commit to the same branch (and rebase if it'd make you happy).

what about failed tests?

those weren't your fault. i restarted the build and am seeing 💚 now.

message: 'An unknown error occurred'
}
};

callback.call(context, err);
window._EsriLeafletCallbacks[callbackId] = true;
}
};
DomUtil.addClass(script, 'esri-leaflet-jsonp');

callbacks++;
Expand Down