-
Notifications
You must be signed in to change notification settings - Fork 78
Initial implementation (& discussion) #2
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
Conversation
Looks nice to me. It's nice to see Thinking out loud, maybe something like Have you considered using something like Maybe a bunch of newtypes and a type class would be a nice way to handle the various content types? For the response, something like |
Even though I raised the point originally in the API discussion I became less concerned about setting the method or URL more than once - I figured the compositional advantage of not restricting that might be worth it. For example, you might want to make most of a request in one place, and then override the method or endpoint later on before running it, while keeping headers or content or something. I did end up putting something in for the content types, but it depends on these additions to JSON isn't a "native" type for XHR, but it probably would be nice to have a combinator that handles that, but then I don't know what type to use for that either! Could be I hadn't even thought of
|
| FormDataContent FormData | ||
|
||
-- TODO: probably not this? Do we want to deal with other responses, include headers, etc? | ||
newtype AjaxResponse = AjaxResponse String |
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.
Yeah, we'll somehow want to support responseBody
, responseText
, and responseXML
, since I think those are the 3 baked into XMLHttpRequestObject
, together with headers and status code. If this is just a record, it's easy enough for users to extract out the appropriate field of interest, so including "too much information" (e.g. headers) is probably OK.
there is an option to set expected type foreign import data RawResponse :: *
class Response a where
to :: RawResponse -> a
instance rJson :: Response Json where
...
-- then add response type to AffjaxRequest Or we can use data Response a = ApplicationJson String | XML String | RawText String and check response type in ffi function |
@cryogenian The 2nd approach won't work, because the client has to "request" the type of response -- the browser will lazily compute it if possible (for example, if you request XML, it will be parsed as XML, but not if you request text). The 1st approach might work for this. Then users call the |
Would it be possible to just tag the request and response types with the type of the response data to ensure that they're compatible? So a |
The request content type has no connection to the response unfortunately as the request content-type header does not determine the response. They don't even have the same set of potential types (supposedly there is an XHR JSON response, although I don't know how you're supposed to get one as |
Sorry, I was thinking of the Accept header. |
Oh, maybe I misunderstood too, |
Yes - maybe it doesn't make sense, I've been kind of dipping in and out of the conversation. But it seems to make sense to be able to say "this is what I expect in the response, fail if it's anything else". You can handle it when you get the response using |
Yeah, request and response types can't be tied together, but it would be nice to tie request / response encoders & decoders to mime types, so you can get those errors like @paf31 mentioned. |
|
👍 Could make it |
^^ Oh, or that :) |
I'll try and address the remaining response-related things tomorrow, I've made the
|
Yes, the data Header = Unknown String String -- or data Header = Header String String A well-done, type-safe HTTP library would be useful here as well as for any node.js HTTP libraries. Oh, forgot to mention another problem with |
I'm having some difficulty with switching over to using
then However, if I keep |
One option would be to make At first I wanted to implement Currently |
Ok, found a middle ground. I introduced a foreign The |
I'm starting a new branch, I think using |
I haven't tested this at all yet, so it's definitely not ready, but there are a few things I thought it'd be worth talking about at this point.
I spent a while trying things out for point 3, including a couple of ways of implementing it without using
purescript-free
, but it did seem silly not to take advantage of what we already have there. However, I'm acutely aware of the amount of machinery being used to enact a pretty trivial task. But then I guess it doesn't have to be super fast, as it's unlikely that people will need to make a lot of requests together, and even if they do theAjaxRequest
type is still available to be used directly if so desired.Any feedback is welcome, but I am intending to check things over and actually try running the code before calling it done! So really it's more the questions about the implementation that I'm looking for input on right now.
Resolves #1