ethul / purescript-angular

AngularJS 1.2 bindings for PureScript (currently in the experimental stage)
MIT License
23 stars 3 forks source link

http Maybe config type #7

Closed wcbeard closed 10 years ago

wcbeard commented 10 years ago

In the $http module, the get function calls $http.get(url, config.values[0]), where I believe config :: Maybe (Config a b). I get an error when I try and call it in

controller scope http = do
  res <- get "http://www.google.com" Nothing http

The complete type is foreign import get :: forall e a b c . Url -> Maybe (Config a b) -> Http -> Eff (nghttp :: HTTP | e) (Promise (Response a b c)). Should it be doing config.values[0], or am I Doing it Wrong?

wcbeard commented 10 years ago

Also, why is Config parameterized by a and b?

edit: Nevermind, it's for params and data

wcbeard commented 10 years ago

Also, am I misunderstanding purescript, or does module ... Method(), mean that GET, POST, etc aren't exported? If so, how should we use them to construct Config's?

ethul commented 10 years ago

hi @d10genes, thanks for catching these. Angular.Http still needs work. I started fixing this up yesterday, but I probably won't push up a commit until this weekend.

Regarding the get function, it relies on an older purescript version. I need to update this to instead use the maybe function instead of trying to access .values (since it does not exist anymore).

Also, you're right that Method() needs to be Method(..). This was a mistake on my part on the last commit I made.

If you need these fixes asap, I should be able to make these changes this evening. Otherwise, I am planning to tackle this this weekend.

wcbeard commented 10 years ago

I don't need them ASAP; worst case I'll just do it locally. If fromMaybe is imported in the file, I'm assuming I can use that in the foreign import get function definition? If not I guess I'll just wait and see what your solution is :)

ethul commented 10 years ago

Sounds good. I will get to it this weekend (hopefully tonight though).

For the fix, I am planning to pass in the maybe function like how it's done for watch, or the fromMaybe function would work to.

Also, I am planning to rework the Config type a bit. For example, angular defines that the data property can be a string or an object. Also, data is optional since it does not apply to all requests. So I was thinking of a type like forall a. Maybe (Either String a).

I'd be happy to hear any feedback on this, but there are a few properties that might go in a similar direction.

wcbeard commented 10 years ago

Thanks, I dug through the javascript that psc generated and think I figured out how to use fromMaybe temporarily.

I'm having a hard time getting a good sense of the data argument from the angular docs you linked to, so I'm not really sure when it would be used as a string argument? At the risk of spinning out of control with new types for every single property, I wonder if it would be worth it to define a type for the data property, like data Data = Data a | NoData | DataS String? I'm starting to see how weird it can get defining a well typed interface to a language like JavaScript... :smile:

I'm also wondering if there is an easy way to write a shortcut generator if all the shortcut functions are similar enough, something like

httpShortcut :: Method -> Url -> Config a b -> Http -> Eff ()
get = httpShortcut GET
ethul commented 10 years ago

The Http module work is still on-going, but I pushed up some changes that might resolve the issue you were having.

I like your proposal for the Data data-type. I am current using Maybe (Either String a) but I'd be open to switching it over. Any feedback on 11327525886e3a1b37b3c8185160f674372bbcf5 is most welcome!

wcbeard commented 10 years ago

I'm not familiar with any purescript frameworks or best practices for testing, but would it be worth considering some tests for some assurance that $http and other directives function properly? I have more faith when pure functions type check in purescript, but at least in the current state of this module it appears to be leveraging a lot of unsafe JavaScript functions.

I looked at the frameworks for purescript-react and purescript-Jquery, but didn't see any examples (other than demos like the one here). I take it purescript's testing tools are still maturing?

ethul commented 10 years ago

I agree that testing would be good to have in the library. Since the library is still in the experimental stages, I'd like to nail down the representations a bit more and work out a few ideas before focussing on tests. Though, there is a purescript/purescript-quickcheck which could be a good place to start.

ethul commented 10 years ago

Just pushed more Http updates in a32718a83af2d6eba706da8a23a3ca8bdb70d746

Also, closing this as I believe the original issue has been resolved.