-
Notifications
You must be signed in to change notification settings - Fork 291
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
New Framework Candidate: SC.Task #874
Conversation
Can you give a comparison of Tasks and the existing Statechart framework? They seem to overlap. |
Certainly! I can see the confusion, of course: Both States and Tasks have a hierarchy and grouping methodology, and I borrowed a few conventions from the statechart (most notably the plugin()) method. Furthermore, Tasks are implemented as a state machine. Conceptually, however, they're very very different. States are there to maintain state - tasks are there to execute logic. In practice, we use States and Tasks cooperatively - States drive our UI, while tasks drive our business logic. Here's a state that might explain the purpose... Editor.ContentEditorState = SC.State.extend({
representRoute : "editor/:id",
enterState : function(context) {
// Derive the ActivityId from the context.
var activityId = context.getPath('params.id');
// Setup the view.
this.setPath('mainView.nowShowing', "Editor.LoadingView");
// Load our data - let's wait for it
return this.performAsync("_delayForPreloader", activityId);
},
_delayForPreloader : function(activityId) {
var task = Editor.PreloadActivityTask.create({
'activityId' : activityId
});
SC.Event.add(task, SC.TaskEvent.COMPLETE, this, '_onTaskPreloaded');
},
_onTaskPreloaded : function() {
// Remove the event listener
SC.Event.remove(task, SC.TaskEvent.COMPLETE, this, '_onTaskPreloaded');
// Setup the view.
this.setPath('mainView.nowShowing', "Editor.ContentEditorView");
this.resumeGotoState();
},
});
Editor.PreloadActivityTask = SC.SequentialTaskGroup.extend({
activityId : null,
tasks : [ "verifyPermissions", "loadActivity", "preloadTranslationStrings" ],
verifyPermissions : SC.Task.plugin("Editor.VerifyPermissionsTask", {
permission : 'mayEdit'
}),
loadActivity : SC.Task.plugin("Editor.LoadActivityTask"),
preloadTranslationStrings : SC.Task.plugin("Editor.LoadTranslationTask", {
language : 'en-US'
}),
startTask : function() {
this.setPath('loadActivity.activityId', this.get('activityId'));
this.setPath('preloadTranslationStrings.activityId', this.get('activityId'));
sc_super();
}
}); Some other notes:
|
Still sounds like stuff that should be in a statechart. I'm not sure I see the benefit yet. Evin Grano On Jan 2, 2013, at 3:45 PM, Michael Krotscheck notifications@github.com wrote:
|
Evin- I've annotated my comment with a code sample earlier today. The key is that states can't be reused, while tasks can. Unless I'm totally missing how y'all are using statecharts. |
Code in statecharts is generally reused through hierarchical separation or parallelism. I've never had a reuse problem with statecharts. I could be miss understanding your tasks though. The idea of setting up code and throwing it away is intriguing. I can see this being useful for business logic that is either internal view-centric or with some controller-centric BL... I like the API. Evin Grano On Jan 2, 2013, at 9:05 PM, Michael Krotscheck notifications@github.com wrote:
|
Yeah, I think we might be talking past each other. Tasks are basically an OO formalization of the Command Queue and Command Design Patterns, with a little Memento pattern thrown in (if you want it) and the caveats that the receiver is different on a per-task basis (by implementation), and that your invoker API is event-based. Or, in its simplest form, it's just a way to batch execute a bunch of functions, some of which you'd like to reuse elsewhere. |
This is unrelated to statecharts, it's just a lightweight way to set up sequential and parallel tasks - like if these three things have to all happen, and might happen in any order, before the fourth thing happens. Otherwise you'll be setting and checking flags, which makes things a little more fragile than they need to be. It's some syntactic sugar and it helps streamline code and remove flags in certain situations. It's similar to the node module Step. Assuming this works as advertised (and is as lightweight as advertised) I'm all for it. I'd change some of the syntax though - I'd like to apply complete and error events directly to the task rather than having to go through class methods. Thoughts? |
agreed. This framework sounds great!! I've been looking for neater ways of structuring functionality out of controllers and states. Would this be suitable to include promises style functionality or is this too much of a different thing? -tc |
Including the class methods was intentional- it gives us the freedom to refactor/fix under the hood at a later time, without forcing anyone to go through a painful upgrade process. While I agree that removing them is convenient, I feel that keeping the engineer-to-framework contract intact is more maintainable in the long run. |
What's the difference between
and
in terms of maintainability and the engineer-to-framework contract? Do you mean that you're thinking of implementing instance methods for this later on, as convenience wrappers to the class method, once things are farther along and you've settled on a syntax? For now, needing to loop back to a class method in order to do something "afterwards" (a basic feature) seems badly undiscoverable. |
Oh, I see what you're getting at. What I understood from your earlier comment was "When I'm building a new task, I don't want to have to call complete() from within my startTask() method, I'd rather just dispatch my own complete event". As far as actually listening to the start/finish/error events on a task, creating the abstraction you're recommending isn't a bad idea, as long as it's coupled with a way to detach the task from the listeners. We must have a root-path reference to a task so that GC doesn't pick it up while it's running, but eventually we want to destroy that reference so we can throw it away. If we do something like you're proposing, we can explicitly add a clearListeners() method (for restartable tasks), and also invoke it from inside the destroy() method. |
Gotchya! No I love it when events get thrown for me. What if the instance methods just passed arguments through to the class methods? Then you could do your cleanup however you wanted in the class methods. On the other hand, if you're going to add listeners you have to be able to remove them anyway (and tidy up on destroy), so no reason not to go the route you describe anyway. |
cleanup method to task groups to ensure that child tasks are cleaned up on destroy.
There ya go! I've gone ahead and used the addEventListener/removeEventListener naming already established by DOM elements, and since it's something I expect others might like to use I've wrapped it into a Mixin: SC.EventSupport |
Nice! |
Hey Michael, This is really wonderful stuff! I also personally feel that all the tasks that I imagine I would use this for are already handled by application states, but I still want this new addition because I can see how it will make my state code much easier to follow (by separating all the long state tasks into separate files like you showed above). I was just about to bring this in, but because there is already an SC.Task class in the framework, I have a question: Does replacing the current SC.Task class with your new version still work with the current SC.TaskQueue/SC.backgroundTaskQueue? I'm guessing it doesn't, but that would be problematic because there's a fair bit of code that is based on the current SC.Task including stuff like working with the app cache and preloading bundles. Please let me know, because I'd rather just add this as an additional regular framework, not tucked into |
Since this pull request went in my company was acquired by an org that has different policies on Open Source. I have to check with the legal department before I can make requested changes. I'll keep you informed. |
Best of luck with the new boss, and keep us posted. (Sincere hopes that we can still get some of your time on the clock!) I assume this pull request is fair game to build off of either way? @publickeating I'm one of the ones that isn't a fan of the current framework setup, but I wanted to clarify: rather than having "fewer frameworks" I would like to have "frameworks split up in such a way that there's a plausible, stated use case for the inclusion or exclusion of each one". Not sure I've stated that anywhere concretely yet. |
I would love to have access to a great control flow library, but the SC.Task name conflict is a blocker. A possible solution is to move this into its own namespace called Tasks. This would leave SC.Task alone and separate from Task.TaskEvent et cetera. I think this is likely to end up being our approach to other frameworks too, like WYSIWYG. |
Closing in favor of updated pull request tackling SC.Task name conflict. @krotscheck if you're out there, this is truly beautiful work. Thanks. |
The SproutCore Task Framework was built to simplify and encapsulate common business logic into easily testable, reusable components. Rather than build a large amount of custom logic in each controller, some of which may be reusable, you instead construct a series of small, atomic, configurable tasks.
Because of the generic set of events that are fired by the Task Framework, you can chain multiple different tasks to run either sequentially or in parallel, and again chain those Task Groups to run sequentially or in parallel as well. Tasks may even be taught how to rewind themselves, so that your application can easily and quickly recover from unexpected error conditions.
NOTE: The Task framework was designed to have a light memory footprint - task instances are expected to have a very short lifespan. As such, SproutCore best practices which construct memory pointers - such as Bindings - are not suited for use inside of tasks as they could cause memory leaks.
Let us take a login flow as an example. When a user logs in, the following steps have to occur:
Using the task framework, this might look as follows: