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

[FEATURE ember-routing-didTransition-hook] Add a `didTransition` hook to the router. #3452

Merged
merged 1 commit into from Sep 26, 2013

Conversation

@mixonic
Copy link
Member

mixonic commented Sep 20, 2013

For use in analytics and such, you may listen to this hook.

Ember.Application.initializer({
  name: 'analytics',
  initialize: function(container, application){
    var router = container.lookup('router:main');
    router.on('didTransition', function(){
      _gaq.push(['_trackPageview', router.get('url')]);
    });
  }
});

Seems good for analytics, though perhaps you could just observe url directly? I imagine there are other use-cases.

@machty
machty reviewed Sep 20, 2013
View changes
packages/ember-routing/lib/system/router.js Outdated
@@ -52,7 +52,7 @@ Ember.Router = Ember.Object.extend({
this.handleURL(location.getURL());
},

didTransition: function(infos) {
finalizeTransition: function(infos) {

This comment has been minimized.

Copy link
@machty

machty Sep 20, 2013

Member

this is risky; I think there's a good number of apps that have reopened didTransition (and called _super) to hook into this event. Perhaps the right answer is branching to finalizeTransition within didTransition if the feature flag is enabled? Does didTransition need to be renamed at all?

This comment has been minimized.

Copy link
@mixonic

mixonic Sep 20, 2013

Author Member

Ah, yes. We meant to Em.K the didTransition function so it could still be subclassed. Will update this.

… to the router.

For use in analytics and such, you may listen to this hook.
@mixonic
Copy link
Member Author

mixonic commented Sep 20, 2013

@machty right you are, there is not need to rename this.

expect(3);

Router.map(function(){
this.route("nork");

This comment has been minimized.

Copy link
@machty

machty Sep 20, 2013

Member

a man from my own heart

stefanpenner added a commit that referenced this pull request Sep 26, 2013
[FEATURE ember-routing-didTransition-hook] Add a `didTransition` hook to the router.
@stefanpenner stefanpenner merged commit a2c0d6e into emberjs:master Sep 26, 2013
1 check passed
1 check passed
default The Travis CI build passed
Details
@mixonic mixonic deleted the mixonic:didTransitionHook branch Sep 26, 2013
if (Ember.FEATURES.isEnabled("ember-routing-didTransition-hook")) {
// Put this in the runloop so url will be accurate. Seems
// less surprising than didTransition being out of sync.
Ember.run.once(this, this.trigger, 'didTransition');

This comment has been minimized.

Copy link
@gpoitch

gpoitch Nov 8, 2013

Contributor

@mixonic how about passing the infos param here?

This comment has been minimized.

Copy link
@mixonic

mixonic Nov 8, 2013

Author Member

@gdub22 looks possible! Out of curiosity, what is in infos that you want access to? What would this enable?

This comment has been minimized.

Copy link
@gpoitch

gpoitch Nov 8, 2013

Contributor

@mixonic this same infos that gets passed into the router's didTransition method (line 55). Just a matter of convenience to have that data accessible in the event callback too.

@huafu
Copy link
Contributor

huafu commented Jan 27, 2014

I don’t see this FEATURE in the list of features, neither I can find didTranstion in the doc, is it activated by default now? Or has it been removed?

@mixonic
Copy link
Member Author

mixonic commented Jan 27, 2014

@huafu
Copy link
Contributor

huafu commented Jan 27, 2014

Ok, thanks ;-) So no need for the feature flag anymore :)

Huafu Gandon | gmail (mailto:huafu.gandon@gmail.com) | github (https://github.com/huafu) | linkedin (http://www.linkedin.com/in/pgandon)

On Monday 27 January 2014 at 07:16, Matthew Beale wrote:

@huafu (https://github.com/huafu) it is in and enabled: https://github.com/mixonic/ember.js/blob/master/packages/ember-routing/lib/system/router.js#L124 The docs are lacking here.


Reply to this email directly or view it on GitHub (#3452 (comment)).

@rwjblue
Copy link
Member

rwjblue commented Jan 27, 2014

@huafu - Exactly, once the feature is shipped in a a release, we remove it from the FEATURES.md and features.json (as it is always on).

@huafu
Copy link
Contributor

huafu commented Jan 27, 2014

Yes I figured out, but what if you decide to finally remove totally the feature? Where are you keeping track of what feature is integrated when?

Huafu Gandon | gmail (mailto:huafu.gandon@gmail.com) | github (https://github.com/huafu) | linkedin (http://www.linkedin.com/in/pgandon)

On Monday 27 January 2014 at 07:19, Robert Jackson wrote:

@huafu (https://github.com/huafu) - Exactly, once the feature is shipped in a a release, we remove it from the FEATURES.md (http://FEATURES.md) and features.json (as it is always on).


Reply to this email directly or view it on GitHub (#3452 (comment)).

@rwjblue
Copy link
Member

rwjblue commented Jan 27, 2014

@huafu - They are tracked in features.json (with descriptions in FEATURES.md) in the root of the main repo. Once a feature is shipped in a release version, the feature flags are removed, and the feature is simply part of the main build.

These might provide more information also:

http://emberjs.com/guides/configuring-ember/feature-flags/
http://emberjs.com/guides/contributing/adding-new-features/

@huafu
Copy link
Contributor

huafu commented Jan 27, 2014

So there is never a feature being abandoned and removed from that?

Huafu Gandon | gmail (mailto:huafu.gandon@gmail.com) | github (https://github.com/huafu) | linkedin (http://www.linkedin.com/in/pgandon)

On Monday 27 January 2014 at 07:43, Robert Jackson wrote:

@huafu (https://github.com/huafu) - They are tracked in features.json (with descriptions in FEATURES.md (http://FEATURES.md)) in the root of the main repo. Once a feature is shipped in a release version, the feature flags are removed, and the feature is simply part of the main build.
These might provide more information also:
http://emberjs.com/guides/configuring-ember/feature-flags/
http://emberjs.com/guides/contributing/adding-new-features/


Reply to this email directly or view it on GitHub (#3452 (comment)).

@rwjblue
Copy link
Member

rwjblue commented Jan 27, 2014

Basically no. Once a feature has been enabled and published in the release version (a final non-beta release) it is considered public API and is then bound by the same SemVer requirements as any other piece of the code base.

Up until it is enabled and released, it can be removed (and this has happened once or twice).

@huafu
Copy link
Contributor

huafu commented Jan 27, 2014

Ok, thanks a lot for all those informations

Huafu Gandon | gmail (mailto:huafu.gandon@gmail.com) | github (https://github.com/huafu) | linkedin (http://www.linkedin.com/in/pgandon)

On Monday 27 January 2014 at 07:48, Robert Jackson wrote:

Basically no. Once a feature has been enabled and published in the release version (a final non-beta release) it is considered public API and is then bound by the same SemVer requirements as any other piece of the code base.
Up until it is enabled and released, it can be removed (and this has happened once or twice).


Reply to this email directly or view it on GitHub (#3452 (comment)).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.