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

Conversation

Projects
None yet
8 participants
@bestcroftington
Contributor

bestcroftington commented Sep 7, 2012

Allows multiple contexts to be passed to Ember.State.transitionTo either as multiple arguments or as an array of contexts within an event.

Changed StateManager.send and sendRecursively to allow multiple context arguments.

Added missing test coverage for transitionTo.

@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Sep 7, 2012

Member

I think this is a dup of #1238

Member

trek commented Sep 7, 2012

I think this is a dup of #1238

@bestcroftington

This comment has been minimized.

Show comment
Hide comment
@bestcroftington

bestcroftington Sep 7, 2012

Contributor

@trek Yep, looks like it is, shame we didn't see that first, would have saved us a while tracking it down!

There is test coverage for this one and a simpler implementation.

We've pushed again to solve the build issues.

  • We've removed type checking of the jQuery event argument. We can't think of a reason why it is necessary
  • If there is no context, then undefined can be passed through without side-effects.
  • It looks like contexts property always exists on the jQuery event so in the case of a single context we can extract it from the contexts array.
Contributor

bestcroftington commented Sep 7, 2012

@trek Yep, looks like it is, shame we didn't see that first, would have saved us a while tracking it down!

There is test coverage for this one and a simpler implementation.

We've pushed again to solve the build issues.

  • We've removed type checking of the jQuery event argument. We can't think of a reason why it is necessary
  • If there is no context, then undefined can be passed through without side-effects.
  • It looks like contexts property always exists on the jQuery event so in the case of a single context we can extract it from the contexts array.
@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Sep 7, 2012

Member

@bestcroftington I was holding off to hear from Yehuda if my assumption about contexts always being present was true.

The type checking is required because not all transitons are the result of user events, so sometimes the second argument is already an Ember object and sometimes that argument is stashed in an event that get's passed.

Multiple contexts need to passed as multiple arguments, not as a single array to work consistently ( i.e. stateManager.transitionTo(target, context) would break in certain cases).

I used

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

I should push my latest

Member

trek commented Sep 7, 2012

@bestcroftington I was holding off to hear from Yehuda if my assumption about contexts always being present was true.

The type checking is required because not all transitons are the result of user events, so sometimes the second argument is already an Ember object and sometimes that argument is stashed in an event that get's passed.

Multiple contexts need to passed as multiple arguments, not as a single array to work consistently ( i.e. stateManager.transitionTo(target, context) would break in certain cases).

I used

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

I should push my latest

@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Sep 7, 2012

Member

or you can update the PR to match those cases.

Member

trek commented Sep 7, 2012

or you can update the PR to match those cases.

Fix State.transitionTo to handle multiple contexts
Allows multiple contexts to be passed to Ember.State.transitionTo either as multiple arguments or as an array of contexts within an event.

Changed StateManager.send and sendRecursively to allow multiple context arguments.
@bestcroftington

This comment has been minimized.

Show comment
Hide comment
@bestcroftington

bestcroftington Sep 14, 2012

Contributor

@trek We've update the PR now and added in the type checking. We've also fixed the case where transitionTo is called without with context arguments rather than an event.

It turns out that this also required some changes to StateManager.send and StateManager.sendRecursively to allow them to pass through multiple contexts.

Now things like:

router.send('showComment', post, comment)

will work.

There is test coverage for each of the transitionTo cases.

Contributor

bestcroftington commented Sep 14, 2012

@trek We've update the PR now and added in the type checking. We've also fixed the case where transitionTo is called without with context arguments rather than an event.

It turns out that this also required some changes to StateManager.send and StateManager.sendRecursively to allow them to pass through multiple contexts.

Now things like:

router.send('showComment', post, comment)

will work.

There is test coverage for each of the transitionTo cases.

@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Sep 14, 2012

Member

Perfect. I'll close my PR in favor of this one.

Member

trek commented Sep 14, 2012

Perfect. I'll close my PR in favor of this one.

@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Sep 14, 2012

Member

@wycats can you give this a look?

Member

trek commented Sep 14, 2012

@wycats can you give this a look?

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus Sep 15, 2012

Contributor

For the record. I'm using this patch and it works fine for me.

Contributor

drogus commented Sep 15, 2012

For the record. I'm using this patch and it works fine for me.

Show outdated Hide outdated packages/ember-states/lib/main.js Outdated
@@ -181,8 +181,6 @@ Ember.State = Ember.Object.extend(Ember.Evented,
exit: Ember.K
});
var Event = Ember.$ && Ember.$.Event;

This comment has been minimized.

@wagenet

wagenet Oct 8, 2012

Member

Bring this line back.

@wagenet

wagenet Oct 8, 2012

Member

Bring this line back.

This comment has been minimized.

@trek

trek Oct 10, 2012

Member

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

@trek

trek Oct 10, 2012

Member

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

Show outdated Hide outdated packages/ember-states/lib/state.js Outdated
@crofty

This comment has been minimized.

Show comment
Hide comment
@crofty

crofty Oct 9, 2012

Contributor

@wagenet You suggest the following change

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

and then changing the type checking to

(Event && contextOrEvent instanceof Event)

isn't this just hiding the dependency rather than removing it?

My issue with this relates to the tests. If the require of ember-views is removed then, in the test suite, Event will always be undefined and so we won't be able exercise the code path when the second argument to transitionTo is actually an event.

There are the tests we added that test the event code path:

I can't think of way that we can continue to test these whilst hiding the dependency on ember-views but please let me know if I am missing something.

In an earlier version of this code we removed the type checking of event but then @trek explained why it is actually required.

Maybe it is worth considering what @trek and @drogus suggested and actually make it so that an event is never passed to transitionTo. This way, no type checking would be required and dependency on ember-views could actually be removed. I'm not sure if this would have impacts on other areas though?

Contributor

crofty commented Oct 9, 2012

@wagenet You suggest the following change

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

and then changing the type checking to

(Event && contextOrEvent instanceof Event)

isn't this just hiding the dependency rather than removing it?

My issue with this relates to the tests. If the require of ember-views is removed then, in the test suite, Event will always be undefined and so we won't be able exercise the code path when the second argument to transitionTo is actually an event.

There are the tests we added that test the event code path:

I can't think of way that we can continue to test these whilst hiding the dependency on ember-views but please let me know if I am missing something.

In an earlier version of this code we removed the type checking of event but then @trek explained why it is actually required.

Maybe it is worth considering what @trek and @drogus suggested and actually make it so that an event is never passed to transitionTo. This way, no type checking would be required and dependency on ember-views could actually be removed. I'm not sure if this would have impacts on other areas though?

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Oct 9, 2012

Member

@crofty what this does is make it an "optional dependency". Basically, it says that if we have Ember.$.Event then we do certain things. If we don't have it, that's ok too. The PR, as it stands, requires that Ember.$.Event always exists. This is not acceptable since ember-states is otherwise useable without ember-views. I don't not want to add a strict dependency. Since your concern is testing, you should be able to add the require only to the tests.

Member

wagenet commented Oct 9, 2012

@crofty what this does is make it an "optional dependency". Basically, it says that if we have Ember.$.Event then we do certain things. If we don't have it, that's ok too. The PR, as it stands, requires that Ember.$.Event always exists. This is not acceptable since ember-states is otherwise useable without ember-views. I don't not want to add a strict dependency. Since your concern is testing, you should be able to add the require only to the tests.

Remove ember-views dependency from ember-states
Ember-views is now an optional dependency for ember-states.  It is only required in the test file.
@crofty

This comment has been minimized.

Show comment
Hide comment
@crofty

crofty Oct 9, 2012

Contributor

@wagenet thanks.
I've removed the dependency and just required ember-views in the test file. I had to move Event = Ember.$ && Ember.$.Event into the transitionTo function so that it is not evaluated before the test file requires ember-views. I assume this is ok?

The travis build is failing due to it not being able to find the ember-views that I am requiring in the test file.
Is there a way to specify a dependency so that it is only included in the tests?

Contributor

crofty commented Oct 9, 2012

@wagenet thanks.
I've removed the dependency and just required ember-views in the test file. I had to move Event = Ember.$ && Ember.$.Event into the transitionTo function so that it is not evaluated before the test file requires ember-views. I assume this is ok?

The travis build is failing due to it not being able to find the ember-views that I am requiring in the test file.
Is there a way to specify a dependency so that it is only included in the tests?

@bestie

This comment has been minimized.

Show comment
Hide comment
@bestie

bestie Oct 11, 2012

Contributor

@wagenet I'm definitely on board with this solution for removing ember views as a 'hard' dependency.

+1 to not passing a jQuery event into a transition as @trek suggested

Is there a use-case for passing an event to a transition? I would have assumed events should not be sent to transitions as you can write an action that both handles the event and invokes a transition.

Contributor

bestie commented Oct 11, 2012

@wagenet I'm definitely on board with this solution for removing ember views as a 'hard' dependency.

+1 to not passing a jQuery event into a transition as @trek suggested

Is there a use-case for passing an event to a transition? I would have assumed events should not be sent to transitions as you can write an action that both handles the event and invokes a transition.

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Oct 11, 2012

Member

@bestie, @trek, I agree that it does seem odd to pass events to the state manager. I think the reason why this happens is that the {{action}} helper doesn't know very much about its target and itself sends events. I'm not sure what would be involved in changing this behavior.

Member

wagenet commented Oct 11, 2012

@bestie, @trek, I agree that it does seem odd to pass events to the state manager. I think the reason why this happens is that the {{action}} helper doesn't know very much about its target and itself sends events. I'm not sure what would be involved in changing this behavior.

@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Oct 11, 2012

Member

@wagenet I wouldn't suggest we remove it for 1.0, but perhaps leave it open for discussion for 2.0 when there's a larger community and we can get more people's perspective on it.

Member

trek commented Oct 11, 2012

@wagenet I wouldn't suggest we remove it for 1.0, but perhaps leave it open for discussion for 2.0 when there's a larger community and we can get more people's perspective on it.

Remove ember-views dependency from test suite, stub the Event.
For the purposes of the test suite Ember.$.Event is required to exist.  We can just use an empty function rather than requiring all of ember-views.
@crofty

This comment has been minimized.

Show comment
Hide comment
@crofty

crofty Oct 12, 2012

Contributor

There doesn't seem to be a way to specify a test suite only dependency, so I've removed the require of ember-views from the state package completely.

In order to test the code path that uses events, I've just stubbed Ember.$.Event with an empty function.

This PR should be good to go now.

Contributor

crofty commented Oct 12, 2012

There doesn't seem to be a way to specify a test suite only dependency, so I've removed the require of ember-views from the state package completely.

In order to test the code path that uses events, I've just stubbed Ember.$.Event with an empty function.

This PR should be good to go now.

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Oct 12, 2012

Member

@crofty sounds good to me :)

Member

wagenet commented Oct 12, 2012

@crofty sounds good to me :)

@trek

This comment has been minimized.

Show comment
Hide comment
@trek

trek Oct 12, 2012

Member

Huzzah! @crofty thanks for your work on this... I know we tend to have more back and forth on PRs than many OSS projects.

Member

trek commented Oct 12, 2012

Huzzah! @crofty thanks for your work on this... I know we tend to have more back and forth on PRs than many OSS projects.

@@ -12,7 +12,7 @@
"dependencies": {
"spade": "~> 1.0",
"ember-runtime": "1.0.pre"
"ember-runtime": "1.0.pre",

This comment has been minimized.

@wagenet

wagenet Oct 12, 2012

Member

Whoops, trailing comma.

@wagenet

wagenet Oct 12, 2012

Member

Whoops, trailing comma.

This comment has been minimized.

@wagenet

wagenet Oct 12, 2012

Member

And fixed: 451ef3d

@wagenet

wagenet Oct 12, 2012

Member

And fixed: 451ef3d

wagenet added a commit that referenced this pull request Oct 12, 2012

Merge pull request #1354 from bestcroftington/master
Allow State.transitionTo handle multiple contexts

@wagenet wagenet merged commit 65aa3d6 into emberjs:master Oct 12, 2012

1 check passed

default The Travis build passed
Details
@joliss

This comment has been minimized.

Show comment
Hide comment
@joliss

joliss Oct 15, 2012

Contributor

This now passes spurious event objects as contexts. Luckily the fix is simple. I've opened #1450 to fix it.

Contributor

joliss commented Oct 15, 2012

This now passes spurious event objects as contexts. Luckily the fix is simple. I've opened #1450 to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment