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 asynchronous module initializers. #792

Closed
wants to merge 1 commit into from

Conversation

andru
Copy link

@andru andru commented Nov 23, 2013

This pull request adds asynchronous module initializers using jQuery Deferred objects.

It adds three new properties to Marionette.Module:

_deferReady

This is a jQuery Deferred object intended for internal use by Marionette.Module.

ready

This is a jQuery Promise object which will be resolved or rejected based on the result of any defined asynchronous initializers. It is intended for use as a "public" property.

_initializerPromises

This is a Marionette.Promises instance (see below)


It adds one event and corresponding triggerMethod:

  • ready

The ready event will be triggered when the ready promise has been resolved.

It adds one new method:

  • addAsyncInitializer

This adds an asynchronous initializer. The callback function should return a promise which will be resolved or rejected when complete. If it returns any other value it will be marked as resolved, but in that case a standard initializer should be used anyway.


It adds one new Marionette object

  • Marionette.Promises

This is modelled very closely on Marionette.Callbacks, and indeed there may be some way to combine their functionality, but I didn't want to mess with such a core dependency. Essentially it's the same, but the run method adds the return value of the callback to an array, and returns a new promise. When all items in the array are resolved, the promise will be marked as resolved.

This pull request allows application code (other modules, routers, etc) to wait until all asynchronous module dependencies have resolved before continuing. Eg.
MyModule.ready.done(function(){ /*do something*/ });
or
MyModule.on('ready', fn)
The downside to the event-based approach is that only handlers bound before the event will be notified. By using the ready promise object one can use MyModule.ready.done(fn) without caring whether the module has just started, or was started and resolved long ago.

Additionally, using the promise objects with jQuery.when can define multiple module dependencies
Eg.
$.when(FirstModule.ready, SecondModule.ready).done(function(){ /* do something */ });

//call the callback and add it's return value (should be a promise)
//to the promises stack
//for convenience, the callback is provided with a deferred object
//it can use to resolve/reject a non-promise asychronous dependency
Copy link
Author

Choose a reason for hiding this comment

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

Note that for convenience callbacks are provided with a jQuery deferred object as their second argument. I'm not sure if this is necessary or just overhead...

@eethann
Copy link

eethann commented Dec 8, 2013

Quick note from a user: we are invoking browser browser geolocation in a marionette app and would find this very helpful.

@@ -27,6 +45,12 @@ _.extend(Marionette.Module.prototype, Backbone.Events, {
addInitializer: function(callback){
this._initializerCallbacks.add(callback);
},
// Asynchronous initializers nitializers are run when the module
Copy link
Member

Choose a reason for hiding this comment

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

Small typo -
+ // Asynchronous initializers are run when the module

@samccone
Copy link
Member

this concept is resolved in #845

@samccone samccone closed this Jan 15, 2014
@andru
Copy link
Author

andru commented Jan 16, 2014

@samcone - #845 does not introduce asynchronous initializers. It introduces an initialize function which is called as a constructor much like in views, models etc: useful but completely distinct functionality, and doesn't introduce any asynchronous behaviour.

This pull request allows the definition of asynchronous resources which must be loaded at module start, via initialize functions. These return a promise to the module which they will resolve when they are ready. After all these initializers have resolved their promises, the module then fires a 'done' event, allowing observers both inside and outside of the module to know that it is ready.

@samccone samccone reopened this Jan 16, 2014
@samccone
Copy link
Member

k @andru going to keep open, for more discussion. This is kinda @cobbweb's baby so I will let him take it from here.

Thanks for the work so far!

@andru
Copy link
Author

andru commented Jan 16, 2014

thanks @samccone. @cobbweb if you need a use case or any further rationale for this just give me a shout.

@ccamarat
Copy link
Contributor

I think this is great. jQuery's Promise library supports both async and synch handlers. If you $.when($.noop).then(doSomething);, doSomething will execute immediately. I'd prefer to see addInitializer retrofitted the same way instead of having to call a new addAsyncInitializer operation.

@andru
Copy link
Author

andru commented Jan 17, 2014

Agreed @ccamarat - ideally I don't see the need for a distinction either. If an initializer returns a promise then the module should add it to the stack of initializer promises, firing done when they've all resolved.

The reason it's currently implemented like this is that it requires changes to the Marionette.Callbacks object. Since this is my first contrib I didn't want to mess with a component which is being utilised in different contexts. That said, as far as I can tell Marionette.Promises could be a non-breaking replacement for Marionette.Callbacks.

If there was core support for the idea I'd happily rework it.

@andru andru mentioned this pull request Jan 17, 2014
3 tasks
@paulovieira
Copy link
Contributor

Seems like a very cool improvement for Marionette.Modules.

@andru, could you please set a jsfiddle showing a use case?

@jamesplease
Copy link
Member

👍 for replacing/extending Marionette.Callbacks with this functionality. And I think it's possible to do this in a non-breaking way – you should go for it!

@ccamarat
Copy link
Contributor

Ditto.

@cobbweb
Copy link
Member

cobbweb commented Jan 17, 2014

Yea this sounds ok, would be good to see if we could use Marionette.Callbacks though.

@samccone
Copy link
Member

maybe get this sucker in for the 1.6 release

@andru
Copy link
Author

andru commented Jan 21, 2014

When's the 1.6 release @samccone ? I can try and get these changes in with tests in about a week.

@@ -2,14 +2,20 @@ describe("module start", function(){
"use strict";

describe("when starting a module", function(){
var MyApp, myModule, initializer;
var MyApp, myModule, initializer, async_initializer;
Copy link
Member

Choose a reason for hiding this comment

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

Use camelCase, asyncInitializer

@jamesplease
Copy link
Member

@andru any updates?

@andru
Copy link
Author

andru commented Feb 26, 2014

@jmeas I've unfortunately not been able to work on the project this is connected to recently. I'll be working on it again in about a week, so I should be able to update this pull request then.

@dminkovsky
Copy link
Contributor

Promises here would be great.

An alternative method to the one proposed in the pull request would be like the one in https://github.com/domenic/chai-as-promised. There, a test runner does not advance to Test 2 until the promise returned from Test 1 is resolved or rejected. A bit externally than what's being proposed here, if I'm understanding correctly.

@dmytroyarmak
Copy link
Contributor

I'm on it and I've almost done)
PR will be after few days.

@Anachron
Copy link

Weren't we trying to remove jQuery from the library dependencies? Correct me if I am wrong.

@andru
Copy link
Author

andru commented Mar 27, 2014

Thanks @dmytroyarmak - I've had almost no free time to commit to this. If you need anything from me please just let me know.

@Anachron Is there a an option for an ES5 promises polyfil? The jQuery implementation of Promises is a particularly weird one, but I didn't want to add dependencies when a promise implementation library was already available in an existing dependency.

@jamesplease
Copy link
Member

I think we should use weird jQuery Deferreds.

//cc @samccone @cobbweb @jasonLaster

@jasonLaster
Copy link
Member

Closing this PR so that the discussion can move to #1069.

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.

None yet