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

Allow State.transitionTo handle multiple contexts #1354

Merged
merged 3 commits into from Oct 12, 2012
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/ember-states/lib/main.js
@@ -1,4 +1,5 @@
require('ember-runtime');
require('ember-views');

Copy link
Member

Choose a reason for hiding this comment

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

Why does this have a dependency on ember-views?

Copy link
Contributor

Choose a reason for hiding this comment

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

We put this in because the code uses Ember.$.Event.

Copy link
Member

Choose a reason for hiding this comment

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

The dependency on something Event-like existed previously, this is just bringing it to light. If jQuery === Ember.$ can we update this PR to
var Event = $ && Ember.$.Event and pull? Or should do we need to better delineate State#transitionTo and Route#transitionTo?

Followup Q: I've never needed the jQuery event in a transition. Can we just pass context and skip the events entirely?

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on skipping jquery event, if someone needs a jquery event they can write their own version of transitionTo

Copy link
Member

Choose a reason for hiding this comment

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

Ember States should not depend on Ember Views. We should definitely fix the code to remove this dependency and then merge this commit without that line.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like we fixed this to be Ember.$ && Ember.$.Event so ember-views is not a dependency.

Copy link
Member

Choose a reason for hiding this comment

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

@bestcroftington Can you get rid of this added require?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm not sure how to remove the dependency whilst we still need to check to see if the argument is an event (instanceof Ember.$.Event).

Can you elaborate on how to remove the dependency? Cheers

Copy link
Member

Choose a reason for hiding this comment

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

I've added some other code comments.

/**
Ember States
Expand Down
30 changes: 15 additions & 15 deletions packages/ember-states/lib/state.js
Expand Up @@ -181,8 +181,6 @@ Ember.State = Ember.Object.extend(Ember.Evented,
exit: Ember.K
});

var Event = Ember.$ && Ember.$.Event;

Ember.State.reopenClass(
Copy link
Member

Choose a reason for hiding this comment

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

Bring this line back.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, now take it out again. Put it back. No, out. Back. Perfect!

/** @scope Ember.State */{

Expand Down Expand Up @@ -211,24 +209,26 @@ Ember.State.reopenClass(
@static
@param {String} target
*/

transitionTo: function(target) {
var event = function(stateManager, context) {
if (Event && context instanceof Event) {
if (context.hasOwnProperty('context')) {
context = context.context;
} else {
// If we received an event and it doesn't contain
// a context, don't pass along a superfluous
// context to the target of the event.
return stateManager.transitionTo(target);
}

var transitionFunction = function(stateManager, contextOrEvent) {
var contexts, transitionArgs;

if (contextOrEvent && (contextOrEvent instanceof Ember.$.Event) && contextOrEvent.hasOwnProperty('contexts')) {
Copy link
Member

Choose a reason for hiding this comment

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

Change this to to (Event && contextOrEvent instanceof Event).

contexts = contextOrEvent.contexts.slice();
}
else {
contexts = [].slice.call(arguments, 1);
}

stateManager.transitionTo(target, context);
contexts.unshift(target);
stateManager.transitionTo.apply(stateManager, contexts);
};

event.transitionTarget = target;
transitionFunction.transitionTarget = target;

return event;
return transitionFunction;
}

});
30 changes: 24 additions & 6 deletions packages/ember-states/lib/state_manager.js
Expand Up @@ -623,14 +623,24 @@ Ember.StateManager = Ember.State.extend({
*/
errorOnUnhandledEvent: true,

send: function(event, context) {
send: function(event) {
var contexts, sendRecursiveArguments;

Ember.assert('Cannot send event "' + event + '" while currentState is ' + get(this, 'currentState'), get(this, 'currentState'));
return this.sendRecursively(event, get(this, 'currentState'), context);

contexts = [].slice.call(arguments, 1);
sendRecursiveArguments = contexts;
sendRecursiveArguments.unshift(event, get(this, 'currentState'));

return this.sendRecursively.apply(this, sendRecursiveArguments);
},

sendRecursively: function(event, currentState, context) {
sendRecursively: function(event, currentState) {
var log = this.enableLogging,
action = currentState[event];
action = currentState[event],
contexts, sendRecursiveArguments, actionArguments;

contexts = [].slice.call(arguments, 2);

// Test to see if the action is a method that
// can be invoked. Don't blindly check just for
Expand All @@ -640,11 +650,19 @@ Ember.StateManager = Ember.State.extend({
// case.
if (typeof action === 'function') {
if (log) { Ember.Logger.log(fmt("STATEMANAGER: Sending event '%@' to state %@.", [event, get(currentState, 'path')])); }
return action.call(currentState, this, context);

actionArguments = contexts;
actionArguments.unshift(this);

return action.apply(currentState, actionArguments);
} else {
var parentState = get(currentState, 'parentState');
if (parentState) {
return this.sendRecursively(event, parentState, context);

sendRecursiveArguments = contexts;
sendRecursiveArguments.unshift(event, parentState);

return this.sendRecursively.apply(this, sendRecursiveArguments);
} else if (get(this, 'errorOnUnhandledEvent')) {
throw new Ember.Error(this.toString() + " could not respond to event " + event + " in state " + get(this, 'currentState.path') + ".");
}
Expand Down
3 changes: 2 additions & 1 deletion packages/ember-states/package.json
Expand Up @@ -12,7 +12,8 @@

"dependencies": {
"spade": "~> 1.0",
"ember-runtime": "1.0.pre"
"ember-runtime": "1.0.pre",
Copy link
Member

Choose a reason for hiding this comment

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

Whoops, trailing comma.

Copy link
Member

Choose a reason for hiding this comment

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

And fixed: 451ef3d

"ember-views": "1.0.pre"
},

"dependencies:development": {
Expand Down
9 changes: 8 additions & 1 deletion packages/ember-states/tests/state_manager_test.js
Expand Up @@ -383,7 +383,7 @@ test("it sends exit events in the correct order when changing to a state multipl
equal(exitOrder[1], 'exitedOuter', "outer exit is called second");
});

var passedContext, loadingEventCalled, loadedEventCalled, eventInChildCalled;
var passedContext, passedContexts, loadingEventCalled, loadedEventCalled, eventInChildCalled;
loadingEventCalled = loadedEventCalled = eventInChildCalled = 0;

module("Ember.StateManager - Event Dispatching", {
Expand All @@ -393,6 +393,7 @@ module("Ember.StateManager - Event Dispatching", {
anEvent: function(manager, context) {
loadingEventCalled++;
passedContext = context;
passedContexts = [].slice.call(arguments, 1);
}
}),

Expand Down Expand Up @@ -442,6 +443,12 @@ test("it supports arguments to events", function() {
equal(passedContext.context, true, "send passes along a context");
});

test("it supports multiple arguments to events", function() {
stateManager.send('anEvent', {name: 'bestie'}, {name: 'crofty'});
equal(passedContexts[0].name, 'bestie', "send passes along the first context");
equal(passedContexts[1].name, 'crofty', "send passes along the second context");
});

test("it throws an exception if an event is dispatched that is unhandled", function() {
raises(function() {
stateManager.send('unhandledEvent');
Expand Down
173 changes: 173 additions & 0 deletions packages/ember-states/tests/state_test.js
Expand Up @@ -180,3 +180,176 @@ test("the isLeaf property is false when a state has child states", function() {
equal(otherInsideFirst.get('isLeaf'), false);
equal(definitelyInside.get('isLeaf'), true);
});

test("Ember.State.transitionTo sets the transition target", function(){
var receivedTarget,
stateManager,
transitionFunction;

stateManager = {
transitionTo: function(target, context){
receivedTarget = target;
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager);

equal( receivedTarget, 'targetState' );
});

test("Ember.State.transitionTo passes no context arguments when there are no contexts", function(){
var contextArgsCount,
stateManager,
transitionFunction,
event;

event = new Ember.$.Event();
event.contexts = [];

stateManager = {
transitionTo: function(){
contextArgsCount = [].slice.call(arguments, 1).length;
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager, event);

equal( contextArgsCount, 0);
});

test("Ember.State.transitionTo passes through a single context", function(){
var contextArgsCount,
stateManager,
transitionFunction,
event;

event = new Ember.$.Event();
event.contexts = [];

stateManager = {
transitionTo: function(){
contextArgsCount = [].slice.call(arguments, 1).length;
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager, event);

equal(contextArgsCount, 0);
});

test("Ember.State.transitionTo passes through a single context", function(){
var receivedContext,
stateManager,
transitionFunction,
event;

event = new Ember.$.Event();
event.contexts = [{ value: 'context value' }];

stateManager = {
transitionTo: function(target, context){
receivedContext = context;
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager, event);

equal( receivedContext, event.contexts[0]);
});

test("Ember.State.transitionTo passes through multiple contexts as additional arguments", function(){
var receivedContexts,
stateManager,
transitionFunction,
event;

event = new Ember.$.Event();
event.contexts = [ { value: 'context1' }, { value: 'context2' } ];

stateManager = {
transitionTo: function(target){
receivedContexts = [].slice.call(arguments, 1);
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager, event);

deepEqual( receivedContexts, event.contexts);
});

test("Ember.State.transtionTo does not mutate the event contexts value", function(){
var receivedContexts,
stateManager,
transitionFunction,
originalContext,
event;

originalContext = [ { value: 'context1' }, { value: 'context2' } ];

event = new Ember.$.Event();
event.contexts = originalContext.slice();

stateManager = {
transitionTo: function(target){
receivedContexts = [].slice.call(arguments, 1);
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager, event);

deepEqual(event.contexts, originalContext);
});

test("Ember.State.transitionTo passes no context arguments when called with no context or event", function(){
var receivedContexts,
stateManager,
transitionFunction;

stateManager = {
transitionTo: function(target){
receivedContexts = [].slice.call(arguments, 1);
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager);

equal( receivedContexts.length, 0, "transitionTo receives no context");
});

test("Ember.State.transitionTo handles contexts without an event", function(){
var receivedContexts,
stateManager,
transitionFunction,
context1,
context2;

context1 = { value: 'context1', contexts: 'I am not an event'};
context2 = { value: 'context2', contexts: ''};

stateManager = {
transitionTo: function(target){
receivedContexts = [].slice.call(arguments, 1);
}
};

transitionFunction = Ember.State.transitionTo('targetState');

transitionFunction(stateManager, context1, context2);

equal( receivedContexts[0], context1, "the first context is passed through" );
equal( receivedContexts[1], context2, "the second context is passed through" );
});