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

Added support for return select options with promises. #231

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

patrick-radius
Copy link
Contributor

As previously suggested by @sandeepgs3 in #230

This version checks for results being 'thenable' this should, in my understanding, be the common factor in Promises/A, jQuery deferred, and polyfills

Checks for .then function as all promises including jQuery deffered are 'thenable'
@akre54
Copy link
Contributor

akre54 commented Apr 23, 2014

Just to make sure I understand, the api should look something like this:

var View = Backbone.View.extend({
  initialize: function(options) {
    this.someCollection = new SomeCollection;
  },
  bindings: {
    selectOptions: {
      collection: function() { return this.someCollection.fetch(); }
    }
  }
});

or

collection: function() { return $.getJSON('endpointToAnArray.json'); }

right?

@patrick-radius
Copy link
Contributor Author

Yes, right... in theory any function that returns a promise-like ('thenable') object (such as jQuery's deferred) should work.. doesn't matter if the wanted list/collection already existed or will exist in the future...

@patrick-radius
Copy link
Contributor Author

Hmm, i do see a little buggy in it... i first get a dropdown with [object Object], etc... so i think i'm missing an 'else' clause somewhere...

hmm the bug is actually not a bug, because Backbone.Collection.fetch returns in it's promise not a collection but the raw data from the backend...
So in that case a user could wrap the request in it's own promise, or deal with the data directly...
For normal promises it works fine...

@akre54
Copy link
Contributor

akre54 commented Apr 23, 2014

fetch is turtles all the way down.... it proxies to Backbone.sync which proxies to Backbone.ajax which proxies to $.ajax. Whatever is returned there is also returned from $.ajax, i.e. a deferred towing json from the backend (see the second example above). What do you propose for this scenario? I think these are the ones we want to target, yes?

@patrick-radius
Copy link
Contributor Author

I'm not sure if stickit should 'fix' that or let it up to the user...
I think supporting promises is still usefull, i use it now in our project similar like this:
http://tech.adroll.com/blog/web/2013/11/12/lazyloading-backbone-collection-with-promises.html

@akre54
Copy link
Contributor

akre54 commented Apr 23, 2014

Yeah Deferreds are ugly beasts. Can you layout what you envision the API would look like?

@patrick-radius
Copy link
Contributor Author

I'm not sure if i understand your question. Which API should i layout?
The PR still allows for using promises AND Deferreds...
Your example like: collection: function() { return $.getJSON('endpointToAnArray.json'); } is still completely valid. The only thing not directly supported is using fetch on a Backbone.Collection, since it doesn't pass the collection but the raw json data in the promise... but that's more something that should be changed in Backbone itself...
Although i guess that would require a breaking change in Backbone.

@akre54
Copy link
Contributor

akre54 commented Apr 23, 2014

I don't think this needs to be changed on Backbone because it would break a major feature that fetch just returns the ajax deferred and its original data. What if the then function checked if optList was a Collection, and passed optlist.models if so, or promisedList if not?

@patrick-radius
Copy link
Contributor Author

It already checks for getting a collection.. that's supported. the 'issue' is that Backbone.Collection.fetch() does NOT return a collection...
So, the then function could wrap it in a generic Backbone.Collection itself, but then you miss all the parse stuff that a backend needs and the user probably has already defined in a custom extended collection. So, in imho better leave that interpretation to the user. But with this PR stickit support functions that return a promise-like so that users can use it... I'm curious what others have to say about this... if this PR is already usefull, or wait until a better option is available... but for me personally it already saves some work with for example the lazy-loading example i linked to earlier...

@akre54
Copy link
Contributor

akre54 commented May 1, 2014

Wrapping in a generic Collection is backwards for the reasons you mentioned, but I think maybe allowing the user to pass back the collection directly should work:

collection: function() { return this.collection.fetch().then(function() { return this.collection; }); }

Still uglier than just passing a collection instance, but may be required here.

var $group = Backbone.$('<optgroup/>').attr('label', label);
addSelectOptions(optList[label], $group, val);
$el.append($group);
var doAddList = function(listToAdd) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about just addList for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or createSelectOptions like from #203.

@patrick-radius
Copy link
Contributor Author

Wrapping in a generic Collection is backwards for the reasons you mentioned, but I think maybe allowing the user to pass back the collection directly should work:

This should not be needed using normal promises which resolve the collection itself, only for funky implementations like jQuery.
We have in our project for example a load method in our base collection which wraps backbones fetch but returns a proper promise resolving into the collection by itself.
In that case the stickit collection property just becomes (in this example using a mediator, but not always needed) something like:

selectOptions:{
  collection:function () {
       return this.App.request( 'collection', 'specificationCollection' );
  },
  labelPath:'name',
  valuePath:'id'
}

imho, pretty clean, but the beauty is that the <select> will fill it's <options> whether the data was already available or will become available in the future.

What is the call for here? Shouldn't optList.then(doAddList) work here?

the call there was to set the context, i noticed in the callbacks the context is often undefined. This made some things easier.

@akre54
Copy link
Contributor

akre54 commented May 1, 2014

This should not be needed using normal promises which resolve the collection itself, only for funky implementations like jQuery.

Not sure I follow. fetch shouldn't return the collection itself. It refers to the request, which returns the data from the $.ajax call. If you have a separate method that returns the collection that's fine, but that's a Backbone thing, not a jQuery thing.

I think I may've gotten confused about passing the Backbone.Collection as collection. I think we're both on the same page that the user should be doing this in their own app.

If you want to merge in the latest changes from master I'll give this a spin. Thanks for the pull.

@akre54
Copy link
Contributor

akre54 commented May 1, 2014

the call there was to set the context, i noticed in the callbacks the context is often undefined. This made some things easier.

I don't think this is doing that. then in most implementations just pushes a callback onto the thenables stack, it doesn't affect the thisArg context of the promised function.

@patrick-radius
Copy link
Contributor Author

Hmm travis timed out... Maybe you could restart the build?

@akre54
Copy link
Contributor

akre54 commented May 1, 2014

You got it.

@gsaandy
Copy link

gsaandy commented May 5, 2014

I think you can add support for 'doneables' as well like 'thenables', as promises like 'q' are implemented 'done' instead of 'then'.

@patrick-radius
Copy link
Contributor Author

As far as i can tell 'q' also offers a then

@akre54
Copy link
Contributor

akre54 commented May 6, 2014

All promise implementations should expose a then. done is mostly for deferreds, as I understand it.

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

Successfully merging this pull request may close these issues.

3 participants