Options based approach

[garyb]

Resolves #1 (maybe!)

• The free monad is gone in favour of using options.
• We now have Requestable and Responsable classes to deal with conversions of values.
  • The XHR contentType property is set based on the expected Responsable value.
  • I still need to do some work with handling the "default" XHR response values as it's all very unsafe right now, I need to figure out a problem I'm having with ArrayBuffers being sent across not coming through with the right length.
  • One disadvantage of this approach is now response values must be explicitly typed or used so they are unified with a type to prevent a No instance for Responsable uXXXX error.
• I need to make simple get, post, etc. combinators.
• We may still want a free monad type thing anyway, as with this options version the headers have to be all set together.
• I need to switch over to mandragora-bucket and integrate the tests into the gulp build.
• unsafeAjax's callback should actually be AffjaxResponse Foreign -> Eff (ajax :: Ajax | e) Unit and then affjax' should read/fromContent the value to actually make it into a Responsable b, handling errors somehow.

[garyb]

I kept working on this and discussed some things with @paf31.

It occurred to me that the current fromContent in Responsable is just reimplementing read from IsForeign, so I made IsForeign a superclass and dropped fromContent. The problem there is, unless we add a purescript-foreign dependency to purescript-dom and purescript-arraybuffer-types and define IsForeign instances there, then they'll be orphans (either that or define instances for those types in purescript-foreign which seems even less sensible).

We can't just make ResponseContent a sum type and define an IsForeign instance for that because we do actually need separate Responsable instances for each of the potentially expected types, as we set the XHR object's responseType property based on the expected result.

I had been thinking that users would be able to specify their own Responsable instances for types they're working with, but actually that's probably only interesting when dealing with JSON responses - it wouldn't really be the job of Foreign to convert an ArrayBuffer or String into a value, those types would have their own parser - so perhaps there still is something in a sum type/psuedo-GADT approach.

I also had some thought about doing away with Requestable and Responsable and having affjax produce a newtyped function with a Profunctor instance, and the input/output could be adapted that way, but that leaves the problem of having to set responseType somehow.

I can't believe how tricky this pretty simple library is proving to be to get right!

[paf31]

What about a scrap-your-typeclasses approach to Responsable? You wouldn't need Proxy and the syntactic overhead would be tiny.

[garyb]

Yes! That would work a lot better, we wouldn't get the missing instance error that way too.

[jdegoes]

@garyb Do you want a review now or wait till you've done the last few things?

[garyb]

I'll push another update to at least address the Responsable stuff, and then it's ready for review. Unless you have any immediate concerns about this approach already, that is!

[garyb]

Ok, this is ready for review now. Unfortunately I ran into a problem with orphan instances for the various requestable types too, as they'd need IsOption instances, so I made affjax accept the RequestContent type directly.

Perhaps the free monad layer on top is even more appealing now, as it would allow us to use a Requestable constraint when setting content to avoid the explicit toContent when using content := ..., as well as being able to set headers individually rather than as an array.

The only outstanding issue I can think of is I need to figure out if there is a way to make the ArrayBuffer responses safe. Or safer at least. The Header, StatusCode, MimeType stuff is pretty rudimentary also, but as we discussed in the other issue, that could be a whole project in itself at some point.

[jdegoes]

In all these methods, I'd put the URL first, because you are likely to make many requests to the same URL, whereas each request is likely to have different content. Also I'd use a type URL = String just for doc purposes.

[jdegoes]

Having to pass in Responsable a is unfortunate. What again was wrong with a type class based approach, e.g.:

class Responsable a where
  mimeType :: MimeType
  fromForeign :: Foreign -> F a

class Requestable a where
  mimeType :: MimeType
  toForeign :: a -> Foreign

[jdegoes]

The advantage of something like this is that it cleans up the APIs, and you can request / respond any suitable type directly.

[garyb]

The issue I had is it would give us orphan instances for Blob, FormData, Document and all the ArrayView types - they need IsOption instances. It also seems like Responsable should be IsForeign a <= Responsable a, as fromForeign is just read from IsForeign, but then we have another orhpan instance situation...

I might revisit the free monad version yet again, maybe there is a way this can be made to work.

[jdegoes]

The issue I had is it would give us orphan instances for Blob, FormData, Document and all the ArrayView types - they need IsOption instances.

This seems like a limitation of the options library (or our use of the library). Also I'd note the only value this library is providing us with is a monoidal syntax for combining optional options. Which is of questionable benefit given all the FFI magic going on. I'd almost prefer the "base" version just use a record with Maybes for everything, that way you could provide a default with Nothing for everything and users could use update syntax to make changes they want. Then other modules could build on that simple dumb version and do things like free monad DSL, options-style monoidal syntax sugar, etc.

I'd just really like the signature to be something like:

affjax :: forall e a b. (Requestable a, Respondable b) => AjaxRequest a -> Aff (ajax :: AJAX | e) (AjaxResponse b)

with affjax function handling setting Content-Type and Accept headers based on the given requestable / respondable (of course, these could be overridden by the user if necessary).

By the way, to some extent I recognize this as bikeshedding, so happy to merge if you like the API as it is. 😄

[garyb]

Yeah, that was basically the approach I took in the other branch but then abandoned after having problems with the DSL. I'll take another look at it and see if I can make the core part work as I agree that the class approach is far more attractive.

[garyb]

Ok, how's this? The classes are back, the options are just a normal record.

I haven't added the content type stuff to Responsable and Requestable as I have a few reservations about that:

• When it comes to Requestable, if you want to use Content-Type: multipart/form-data you also have to be responsible for generating boundary=..., and then making sure that's used correctly when the data is encoded. The browser generally seems to be better at picking Content-Type based on the type of data being sent than I am at least! The option is still there to set it directly using headers if you really know what you're doing, which brings up another point, if we were to take the content type from Requestable would that be overridable by setting it in headers, or would Requestable's content type always take precedence?
• For Responsable, do we actually gain anything by adding this? We set the responseType on XHR already, based on the Responsable type, and then use foreign-reading to ensure we're getting the right result back anyway. I suppose it could be used to set the Accepts header perhaps? Then the same question again, can we override that by setting it in headers?

[jdegoes]

My only comment on mime types is that mime types are much more specific than the 5 types (Document ,XML, JSON, String, etc.) supported directly by XMLHttpRequest. In general, a client should always specify an Accept mime type to indicate precisely which mime types it can process, because a conforming server will use this header to determine which type of content to generate; and a client should always specify Content-Type so the type is unambiguous (e.g. text/csv versus just String).

I'm going to merge this in now, let's do separate PRs for followup work.

[jdegoes]

Btw, can you add travis in here so we can get a CI build going?

[garyb]

Great, sorry it took so long to get here!

Travis is in another PR, I'll open some issues for the other bits.