Skip to content
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

[discussion] API for continuing the traversal #40

Closed
basti1302 opened this issue Mar 30, 2015 · 10 comments
Closed

[discussion] API for continuing the traversal #40

basti1302 opened this issue Mar 30, 2015 · 10 comments

Comments

@basti1302
Copy link
Member

In various issues people have requested the possibility to continue the link traversal process when the initial traversal has finished (#7, #24, traverson-hal#4)

I started to work on continuing traversals in this branch.

The API currently looks like this:

traverson
.from(rootUrl)
.follow('link1', 'link2')
.getResource(function(err, firstResource, traversal) {
  if (err) { return done(err); }
  // do something with the first resource, maybe decide where to go from here.
  traversal
  .continue()
  .follow('link3')
  .getResource(function(err, secondResource) {
    if (err) { return done(err); }
    // do something with the second resource
  });
});

The basic idea is that the callbacks given to the action methods (get, getResource, post, ...) are called with an additional, new parameter which represents the traversal process that has just been finished. This object traversal currently offers only one method, continue() which in turn returns a request builder instance which can be used just as the standard request builder. That is, it has the same configuration and action methods.

Using traversal.continue() to initiate a new traversal ensures that the new traversal is started right where the first traversal stopped and it makes use of the last HTTP response of the first traversal to do so.

This should even enable us to follow multiple relations from a resource because the request builder from traversal.continue() can be cloned with newRequest() as often as you lik to branch of multiple traversal processes from the current resource.

My question: What do you think? Is this a reasonable API for this feature? Any suggestions?

@xogeny
Copy link

xogeny commented Mar 30, 2015

I'd like to see this handled the same way promises are. What I mean by that is that you can start with a common "promise" and then create multiple parallel promises from that. For example:

var api = traverson.from(apiRoot);
var l1 = api.follow('link1')
var l2 = api.follow('link2')
var p1 = l1.getResource().then(function(r1) {
  return l2.getResource().then(r2) {
    return r1.x+r2.x;
  }
});

The point is that this code returns a promise for getResource which means you can chain things together. So if I want to get a property called x from link1 and link2, I can do that. Even better would be to use tricks like spread from promises so that you could fetch an array of resources in parallel to get an array of promises to the resource and then transform the array of promises to resources into a promise for an array of resources.

@jinder
Copy link

jinder commented Mar 30, 2015

I only started using your library yesterday, so I don't have a full grasp of it yet :)

I agree with @xogeny. I'd expect all new JS asynchronous APIs to be only Promise-based, and not support callback functions unless for legacy reasons. The one use case I had yesterday, was a HAL implementation of "streaming" paging. Here, every resource had a unique link to the next page of results. I'd like to be able to do this (ES6 syntax):

traverson
   .from(apiRoot)
   .follow("results")
   .getResource()
   .then((resource, traversal) => {
       // Do something with resource
       return traversal.follow("next").getResponse();
   }).then((resource) => {
      // Do something with the next resource
   });


@xogeny
Copy link

xogeny commented Mar 30, 2015

One thing I didn't really make clear here is that these chains need to be stateless. What I mean by that is that if I do something like this:

var l1 = api.follow('link1').addRequestOptions(...);
var l2 = api.follow('link2').addRequestOptions(...);

It's important that even thought l1 and l2 share a common root (api), what we do on one chain shouldn't bleed over into the other.

@jinder
Copy link

jinder commented Mar 30, 2015

I guess you can't have multiple parameters on a promise resolution. So perhaps the object passed to the continuation function should contain the response and the traversal at that point:

traverson
   .from(apiRoot)
   .follow("results")
   .getResource()
   .then((x) => {
      // where x is { resource, traversal }
   })

@marartner
Copy link

After initially proposing my idea/request to you, I started to build some kind of proof of concept myself, just to see if I could. Its nowhere near production ready, depends on the use of HAL, and only supports GET-Requests for now, but it shows how I thought this could work, which is a bit different from your approach, but not that far away from what xogeny and jinder are talking about here. Take a look, feel free to try it out with the maze API I did there - I hope it helps and inspires in some way. And if you have feedback yourself, shove it my way.

@basti1302
Copy link
Member Author

First of all, thanks to all of you for your feedback <3

Alas, I'm not sure how this has turned into a discussion about promises versus callbacks 😲

Traverson has always been callback based and I don't see a compelling reaon to change that for the continuation feature.

Reasons agains promises from my point of view:

  1. Traverson runs on Node.js. The callback is the agreed-upon async primitive in Node-land. I do not want to suddenly force promises onto all Traverson users and I also do not want this API (continuing traversals) to be promise based while the other features remain callback based (this would be incoherent and confusing)
  2. Traverson runs in the browser. Providing promises would require to add a promises library to the browser build, adding to the file size (which is already too large anyway)
  3. If you want a promise based API, you could always write a thin wrapper for Traverson that promisifies its API. Actually, traverson-angular does quite exactly that, it's only a tiny wrapper around Traverson's API that promisifies each callback-based method. In AngularJS-land, promises are the agreed-upon async "primitive", so it makes total sense to offer a promise based API. (traverson-angular unfortunately is, well, AngularJS-specific because it depends on the AngularJS specific mechanisms, but a similar wrapper could easily be implemented without depending on AngularJS, just using a general purpose promise library like bluebird or q. If someone of you wants a promise-based API so badly that they would actually implement this wrapper I would offer to help with this and also co-maintain it, if necessary

@xogeny: The examples you show are totally possible with the callback based API. And for the statelessness/not-bleed-over-aspect, newRequest() gives you that.

@jinder:

I'd expect all new JS asynchronous APIs to be only Promise-based, and not support callback functions unless for legacy reasons.

That's okay but then your expectations are very different from mine 😉

@amadox: Since you mention the maze API, I also implemented a maze client as a proof of concept on top of the of continue() feature proposed here, see https://github.com/basti1302/traverson/blob/continue-traversal/test/maze.js#L120 -- works pretty well (even without promises 😏)
Feedback regarding your implementation: marartner/travhaller#1

@basti1302
Copy link
Member Author

Oh one more thing just came to my mind... getResource() & co. already return a value, the handle that can be used to abort a traversal process. So returning a promise now instead is a major breaking change.

@jinder
Copy link

jinder commented Apr 1, 2015

It was worth a try :) The continue() feature seems to work for my use case, so I'm happy even if it isn't promise-based!

@basti1302
Copy link
Member Author

This will be a bit awkward now :-( ... I asked for feedback, I got feedback and now it seems I don't even want to listen to the feedback because I'm not in inclined to introduce promises.

Anyway, since nothing else except the promise/callback came up here and also since the discussion basically stopped after my 2 cents, I'm going forward with this. The .continue() method works now and the respective branch has been merged into master.

I think I'll publish a version with this feature soon-ish.

@basti1302
Copy link
Member Author

This has landed in 2.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants