Skip to content

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

Closed
wants to merge 8 commits into from
Closed

Conversation

garyb
Copy link
Member

@garyb garyb commented Mar 14, 2015

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.

  1. How do we want to deal with the various content types that can be sent?
  2. What do we want for the response? Is plain text enough? What about reading response headers?
  3. Is the free monad overkill?

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 the AjaxRequest 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

@paf31
Copy link

paf31 commented Mar 15, 2015

Looks nice to me. It's nice to see Free being used some more.

Thinking out loud, maybe something like RebindableSyntax for >>= would let you constrain things like limiting to one method, or one URL.

Have you considered using something like purescript-options for the AJAX request object?

Maybe a bunch of newtypes and a type class would be a nice way to handle the various content types? newtype PlainText, newtype JSON, etc.

For the response, something like Foreign seems like a decent default for the low-level API, which you could then layer abstractions on top of.

@garyb
Copy link
Member Author

garyb commented Mar 16, 2015

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 purescript-dom - which I don't know if we want to add necessarily, but it seemed sensible to have canonical foreign types for those things.

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 forall a. { | a}, could be Foreign, could be Json from purescript-argonaut etc. so again I left that out for now. Json would be the "best" solution, but pulling argonaut in to this is probably overkill.

I hadn't even thought of purescript-options actually, but yeah, that would probably work.

Foreign probably is the way to go for the response too, given it can be one of a number of types. Perhaps there could be two ways of running a request, one returning Foreign and the other (Foreign, [Header]) or something like that.

| FormDataContent FormData

-- TODO: probably not this? Do we want to deal with other responses, include headers, etc?
newtype AjaxResponse = AjaxResponse String
Copy link
Contributor

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.

@cryogenian
Copy link
Member

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

@jdegoes
Copy link
Contributor

jdegoes commented Mar 16, 2015

@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 to function (hopefully by some better name 😄 ), to convert from the response to something that is provably obtainable from the XMLHttpRequest (i.e. responeBody, responseXML, responseTxt). Argonaut's JSON type could implement such a thing.

@paf31
Copy link

paf31 commented Mar 16, 2015

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 Request Blob would result in a Response Blob or whatever, and then you provide smart constructors to create requests with a finite set of acceptable content types.

@garyb
Copy link
Member Author

garyb commented Mar 16, 2015

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 application/json content-type response headers don't trigger it, along with some moz-specific stuff).

@paf31
Copy link

paf31 commented Mar 16, 2015

Sorry, I was thinking of the Accept header.

@garyb
Copy link
Member Author

garyb commented Mar 16, 2015

Oh, maybe I misunderstood too, AjaxRequest is already parameterised with the send-content-type, did you mean just adding another parameter to cover the responses also?

@paf31
Copy link

paf31 commented Mar 16, 2015

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 foreign, argonaut, or whatever, but this seems to assert a little extra type safety between request and response. It changes the meaning of the errors to "the server is probably doing something wrong".

@jdegoes
Copy link
Contributor

jdegoes commented Mar 16, 2015

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.

@jdegoes
Copy link
Contributor

jdegoes commented Mar 16, 2015

class AjaxRequestable a where
  requestMimeType :: MimeType
  to :: a -> Foreign

class AjaxResponsable a where
  responseMimeType :: MimeType
  from :: Foreign -> Either Error a

@garyb
Copy link
Member Author

garyb commented Mar 16, 2015

👍

Could make it forall req res. (IsForeign res) => AjaxRequest req res perhaps then?

@garyb
Copy link
Member Author

garyb commented Mar 16, 2015

^^ Oh, or that :)

@garyb
Copy link
Member Author

garyb commented Mar 18, 2015

I'll try and address the remaining response-related things tomorrow, I've made the Header and Verb stuff local to affjax now.

Header is just as bad as it was before, if not worse considering I've not provided a set of default HeaderHeads, but I figured for now I'd just go with something simple that will work until we have a proper HTTP library. Maybe this is still wrong though, as I guess to get correctly typed Header values we can't separate the value from the HeaderHead.

@jdegoes
Copy link
Contributor

jdegoes commented Mar 18, 2015

Yes, the Header must be a sum type, where each term encodes the type-safe values possible by its constructor. So maybe the right thing to do in the short term is something quick & dirty, e.g.:

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 http, that is, it doesn't separate RequestHeader from ResponseHeader, and the two are actually quite different; they share a few terms but are conceptually and semantically distinct. Most types of request headers don't make sense in responses, and visa versa.

@garyb
Copy link
Member Author

garyb commented Mar 20, 2015

I'm having some difficulty with switching over to using AjaxRequestable.

  1. If I change the content property to a
  2. create a NoContent value with AjaxRequestable instance
  3. update defaultRequest to use that and be typed as AjaxRequest NoContent

then affjaxRequest doesn't work out anymore, as the type in State is expected be AjaxRequest NoContent.

However, if I keep content as Maybe a it becomes annoying to use, as making a simple call without setting some content results in errors like No instance found for Network.Affjax.Request.AjaxRequestable u15626.

@garyb
Copy link
Member Author

garyb commented Mar 20, 2015

One option would be to make content's type Foreign, but that doesn't seem like a very good idea.

At first I wanted to implement AjaxRequestable's to as being a -> Content, which would also solve this I think, but I ran in to a difficulty there as well.

Currently Content's a parameter is for the ArrayViewContent constructor, I was going to make use Exists ArrayView so we could drop the parameter from Content, but @paf31 pointed out on IRC that allows any kind of nonsense - which is also true with Content a at the moment. Perhaps having ArrayViewContent use a sum type as its argument that restricts the possible set to sensible types is the way to go, although not entirely convenient.

@garyb
Copy link
Member Author

garyb commented Mar 21, 2015

Ok, found a middle ground. I introduced a foreign AjaxContent type, which is basically just Foreign but with more meaning at least.

The AjaxRequestable class has a bunch of default instances for the "low level" content types to allow those to be sent, I'm not 100% sure on the mime types provided for those, but the idea is they will make it easier to produce AjaxRequestable instances for higher level types rather than necessarily be relied on directly.

@garyb garyb closed this Mar 22, 2015
@garyb
Copy link
Member Author

garyb commented Mar 22, 2015

I'm starting a new branch, I think using purescript-options might be a better approach.

@garyb garyb deleted the initial-impl branch April 1, 2015 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

API discussion
4 participants