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.

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

no-callback fix for jsonp with server error #1070

Merged
merged 2 commits into from
Mar 23, 2018
Merged

Conversation

strajuser
Copy link
Contributor

Fix for no-callback jsonp request with server error https://github.com/Esri/esri-leaflet/issues/1069 with onerror handler.

@strajuser
Copy link
Contributor Author

All local tests was Ok.
Sorry about bad username in commit, I can change it in another PR.

Copy link
Contributor

@jgravois jgravois left a comment

Choose a reason for hiding this comment

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

thanks for this contribution!

// 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

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.

@jgravois jgravois merged commit a86cc7d into Esri:master Mar 23, 2018
jgravois pushed a commit to jgravois/esri-leaflet that referenced this pull request Apr 23, 2022
* no-callback fix for jsonp with server error

* jsonp unknown error message update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants