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

Siren actions support #6

Closed
XVincentX opened this Issue Apr 20, 2015 · 34 comments

Comments

Projects
None yet
2 participants
@XVincentX
Contributor

XVincentX commented Apr 20, 2015

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

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Apr 21, 2015

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

This comment has been minimized.

Owner

petejohanson commented Apr 21, 2015

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:

  • By having form access do a copy/clone, we support multiple "fill out and submit a form" without re-requesting the containing resource.
  • Should be fairly easy to databind to a given field, do some trickery with based on type to render different controls, etc.
  • submit() should take care of extracting field values, and passing method, type, and data to underlying http transport.

Potential issues:

  • Is this too "Siren-specific" for a generic client? No forms/actions in HAL. The closest thing in UBER would be a data element w/ the model and action attribute specified, but from reading the spec, I don't see a way to specify the type of a given field.
  • Anything to be done to make this even more databinding friendly?
  • Any commonalities w/ how angular-hy-res might support GET, update field values, PUT type resource updates in the future? Something to factor into our thinking on the API for this or not?

Thoughts?

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Apr 21, 2015

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

This comment has been minimized.

Contributor

XVincentX commented Apr 21, 2015

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

@petejohanson petejohanson self-assigned this Apr 22, 2015

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Apr 22, 2015

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

This comment has been minimized.

Contributor

XVincentX commented Apr 22, 2015

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

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

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Apr 25, 2015

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

This comment has been minimized.

Contributor

XVincentX commented Apr 28, 2015

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 👎
@XVincentX

This comment has been minimized.

Contributor

XVincentX commented Apr 28, 2015

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

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

This comment has been minimized.

Owner

petejohanson commented Apr 28, 2015

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

This comment has been minimized.

Owner

petejohanson commented Apr 29, 2015

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

This comment has been minimized.

Contributor

XVincentX commented Apr 29, 2015

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

This comment has been minimized.

Owner

petejohanson commented May 2, 2015

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

This comment has been minimized.

Owner

petejohanson commented May 2, 2015

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

This comment has been minimized.

Owner

petejohanson commented May 7, 2015

Have you had a chance to give this a try?

@XVincentX

This comment has been minimized.

Contributor

XVincentX commented May 7, 2015

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

This comment has been minimized.

Contributor

XVincentX commented May 21, 2015

@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

This comment has been minimized.

Owner

petejohanson commented May 22, 2015

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

This comment has been minimized.

Contributor

XVincentX commented May 22, 2015

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

This comment has been minimized.

Owner

petejohanson commented Jun 6, 2015

Any other feedback? Working for your needs?

@XVincentX

This comment has been minimized.

Contributor

XVincentX commented Jun 7, 2015

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 😄

@XVincentX

This comment has been minimized.

Contributor

XVincentX commented Jun 11, 2015

@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

This comment has been minimized.

Owner

petejohanson commented Jun 12, 2015

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

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Jun 12, 2015

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

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Jun 12, 2015

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

@XVincentX

This comment has been minimized.

Contributor

XVincentX commented Jun 12, 2015

@petejohanson
It still does not work.

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

image

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Jun 12, 2015

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

This comment has been minimized.

Contributor

XVincentX commented Jun 12, 2015

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

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Jun 13, 2015

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

@XVincentX

This comment has been minimized.

Contributor

XVincentX commented Jun 13, 2015

@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

This comment has been minimized.

Owner

petejohanson commented Jun 13, 2015

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

This comment has been minimized.

Contributor

XVincentX commented Jun 13, 2015

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

@petejohanson

This comment has been minimized.

Owner

petejohanson commented Jun 14, 2015

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

This comment has been minimized.

Contributor

XVincentX commented Jun 14, 2015

Working as expected. Thanks.

@XVincentX XVincentX closed this Jun 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment