petejohanson / angular-hy-res

Hypermedia resource library for AngularJS
MIT License
26 stars 5 forks source link

Siren actions support #6

Closed XVincentX closed 9 years ago

XVincentX commented 9 years ago

I know you're going to hate me, but do you have any plan for that?

petejohanson commented 9 years ago

It actually is one of the line items in the ToDo section at the end of the README. I haven't yet given much thought to what the action API might look like; I need to re-read the Siren spec for inspiration.

petejohanson commented 9 years ago

So... in my mind, Siren's "actions" are really "forms", in the hypermedia sense. I'm envisioning an API like:

var form = resource.$form('create-form'); // *copy* of the form from the resource, since it will be mutated
console.log(form.fields); // [{name: 'email', type: 'email', value: ''}, {name: 'name', type: 'text'}]
console.log(form.field('email')); // {name: 'email', type: 'email', value: ''}
form.submit(); // or... send()?

Features:

Potential issues:

Thoughts?

petejohanson commented 9 years ago

I'll be working on this in the siren_actions branch of hy-res, if you want to comment/follow along:

https://github.com/petejohanson/hy-res/tree/siren_actions

XVincentX commented 9 years ago

Sorry for not answering you yet. I need to refresh my siren knowledge, but I promise I will in following days

petejohanson commented 9 years ago

Ok, I've got a basic implementation started in that branch. Good unit test coverage, but no integration test yet like we have for HAL. Any feedback welcome.

XVincentX commented 9 years ago

It is great. I'll be happy to follow the development of this. I am afraid I will be out for trip until Monday. :-1:

BTW I'd be happy to have a look to an eventual Pull Request you will open!

petejohanson commented 9 years ago

When you return: Check out latest master. Includes Siren integration tests in test/spec/siren_integration_test.js that test GET/PUT/POST, and the few serialization types supported.

XVincentX commented 9 years ago

Hello and sorry for the delay. I've reviewed your thoughts and fundamentally I agree with that.

Let's go thought the issues:

  1. UBER is not the only one that could implement similar stuff. For example, there is HALE as well, and it would be easy to found other 3000 similar formats. For this reason is quite impossible to find the best one. The research is high and formats about hypermedia crank out everyday. For this reason I'd accept the compromise to be a bit siren specific and change that when another format will eventually come to maturity. I think won't take a lot of time to see some extensions with validation informations as well
  2. It looks already databinding friendly. I mean, I can use Angular and I'd know how to use that informations to create my forms.
  3. I am afraid I can't help on this point. I've not dug into source code enough to express an opinion on that :-1:
XVincentX commented 9 years ago

I looked into the code, and looks good. I'd bump it into a new release for angular-hy-res :+1:

I see that you fast forwarded the branch into the master. It makes more difficult to me to understand code changes. Next time it would be great if you could create a Pull Request and merge it, so that a reviewer (me, for example) can easily identify your changes!

petejohanson commented 9 years ago

Next time, I will go the PR route, good idea. Not used to this project actually having some collaboration. Glad to have it.

The only piece I don't love about the current forms code is the return value of Form#submit being a plain $http request/response promise. It feels a bit too low level. In many cases, returning a Resource instance may make the most sense e.g. a POST which creates a new resource. But if a server just returns a 204 No Content, that maps poorly to a resource.

Perhaps we have two methods, Form#submit, and Form#submitResource? One returns bare promise, the other a Resource instance?

Thoughts?

petejohanson commented 9 years ago

Also, I hadn't seen HALE before. I think that is a great second format to consider in terms of commonality and what level of abstraction would work for both formats. Thanks for the link.

XVincentX commented 9 years ago

I would try to avoid as much as possible to have two functions to do the same thing (submit and submitResource)

An empty resource IS a resource, so even if maps poorly, a resource object should be created (in my opinion). Throw exceptions everywhere, but give me the same object!

About HALE, you can definitely find tons of formats like that one, but do not stuck in one of them. Most of them are playground and researches that ends in the /dev/null of the world.

You may also be interested in JSON-API, Hypermedia format comparision.

Have fun, and bump the library on bower!

petejohanson commented 9 years ago

Just published v0.0.16 which bundles hy-res@0.0.10 with forms/siren action support. I've published basic API docs for core hy-res to http://petejohanson.github.io/hy-res/

Give it a try, let me know.

petejohanson commented 9 years ago

Oh, and I did switch the return of Form#submit to be a Resource, FYI. Probably could use some 201 Created automatic redirect foo though, to be useful for that case.

petejohanson commented 9 years ago

Have you had a chance to give this a try?

XVincentX commented 9 years ago

Not yet, but I am really near to try it. I am developing a small clienti with your stuff and won't take a long to use the new form feature.

XVincentX commented 9 years ago

@petejohanson Ok, I was going to give a try to Siren actions but from the current object I am not able to find a submit method or something to send the request (and of course get/set data). Should I handle this by myself? If yes, why are you carrying on an instance of $http?

petejohanson commented 9 years ago

The Form prototype has a "submit" function: https://github.com/petejohanson/hy-res/blob/master/src/form.js#L57 You should be able to invoke form.submit() and get back a Resource instance that will eventually resolve with the response to submitting the action request. Docs: http://petejohanson.github.io/hy-res/Form.html#submit

XVincentX commented 9 years ago

Cool! Sorry, I did not notice the method in the proto. Simple submit are working, I'll try to test with more complicated scened. Thanks.

petejohanson commented 9 years ago

Any other feedback? Working for your needs?

XVincentX commented 9 years ago

Not yet, I am sorry. I am starting to work on this just today, but basically the idea is to make it work together with angular-formly which is easy to setup if you're using a css framework (and I am not at all).

I'll keep you updated, I promise :smile:

XVincentX commented 9 years ago

@petejohanson Hey, I tried finally to submit a form and looks like something is not working properly into the submit method.

In the specific, the transformRequest method is expecting hto be an object, while it is a function, so the transformation does not occur, and the payload is sent directly to the server. In my case, I need my object to be transformed into JSON.

I've attached an image, I hope it may be useful for you.

image

The code that I am using is the simplest possible

    this.action.type = "application/json" //I have to force this, the siren endpoint is not giving me the info
    this.action.field("question").value = this.text
    this.action.field("choices").value = this.choices
    this.action.submit().$promise.then((response) =>{
      debugger
    })

Thanks!

petejohanson commented 9 years ago

I keep forgetting about the angular header API, being a function call with optional parameter. Will get it fixed up soon.

petejohanson commented 9 years ago

Which is the one major incompatibility between angular's $http and axios that keeps biting me.

petejohanson commented 9 years ago

Ok, should hopefully be fixed in angular-hy-res@0.0.20. Please give it a try and let me know.

XVincentX commented 9 years ago

@petejohanson It still does not work.

You're looking for Content-Type key, but the object has got one with lowercase content-type :smile:

image

petejohanson commented 9 years ago

Ugh, OK so angular is lower casing even the headers in the config for the request. Proof I need at least a few integration tests for angular-hy-res, not just core. I'll let you know when I have a new release.

XVincentX commented 9 years ago

Absolutely. Can I hope, in meantime, for a quick hotfix?

petejohanson commented 9 years ago

Not as quick as I would have liked, but hy-res@0.0.14 and angular-hy-res@0.0.21 is published.

XVincentX commented 9 years ago

@petejohanson The form is now correctly submitted. However, it would be definitely useful to add an Accept header, set it with the current hypermedia format (application/vnd.siren+json, in my case) and put it on top of the other. Otherwise, the server may prefer application/json, which is not what I am expecting.

Actually, my request is launched with

application/json text/plain /

petejohanson commented 9 years ago

Current format? Or all formats we have from registered extensions? Maybe with a q qualifier so the format the form was parsed from is preferred?

XVincentX commented 9 years ago

@petejohanson I think the current format should be enough. Btw, you may give the user the ability to configure it.

petejohanson commented 9 years ago

Ok, I did decide to go the route of generating Accept header based on registered extensions, and then the 'preferred response type' can be set and it causes all the other media types in the accept header to have a lower quality value than the preferred one.

Although it may be unlikely, it could be that response types from form submissions are not the same media type as the type that generated the form, so we send our preference, but still include the other media types that we can "understand" if the server send back those instead.

I've released angular-hy-res@0.0.22 and hy-res@0.0.15 w/ the changes.

Fairly soon I'd like to mark this issue as closed and instead prefer to open specific bug/feature request issues on petejohanson/hy-res so we can track changes with a little more granularity. If this last fix gets you basic working Siren actions/forms, let's mark this closed and go from there.

Thanks again.

XVincentX commented 9 years ago

Working as expected. Thanks.