-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat: BadUrlError #166
base: main
Are you sure you want to change the base?
feat: BadUrlError #166
Conversation
TODO: add test to node and web or maybe check my error code? ERR_UNESCAPED_CHARACTERS
|
but returns success in browser even this doesnt throw our error in browser
couldnt reproduce with try around open but it still makes sence to implement this error |
@@ -215,6 +218,8 @@ request driver req = | |||
TimeoutError | |||
else if message == requestFailedMessageIdent then | |||
RequestFailedError | |||
else if message == "Request path contains unescaped characters" then | |||
BadUrlError req.url |
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.
It seems to me like RequestContentError
would work for this case?
Then we would not have to add BadUrlError
and then we would not have to bump the major version.
Description of the change
This pr adds BadUrlError, just like in Elm
https://github.com/elm/http/blob/34a9a27411c2492d3e247ac75cd48e22b473bef5/src/Http.elm#L539
which is thrown during
xhr.open
https://github.com/elm/http/blob/34a9a27411c2492d3e247ac75cd48e22b473bef5/src/Elm/Kernel/Http.js#L33thought I wasn't been able to reproduce this, instead for me on
spago test
it was thrown onxhr.send
the error we are handling is (check first commit srghma@0d30f28)
i'm not sure about this change
Checklist: