Injecting router service into EmberRouter infinitely recurses

[locks]

Code sample:

// app/router.js
const Router = EmberRouter.extend({
  location: config.locationType,
  rootURL: config.rootURL,

  routerService: service('router'),

  init() {
    this.routerService.on('routeDidChange', () => {});
  }
});

Reproduction: https://github.com/ember-triage/emberjs-17791

P.S. Injecting router into EmberRouter also breaks.

[ghost]

Per our discussion on discord Injecting the router works but you must invokethis._super(...arguments);
https://github.com/efx/emberjs-17791/commit/8d8dd5afa1b9841742d1127a95689ab98c085632#diff-f3a289058604b2b069d07bb8e2cda60cR8
My error; conflated this with ensuring you can register routing events from the router instance.

[locks]

@efx that code snippet is not injecting the router service, I believe that is a different situation!

[ursqontis]

I can also confirm this behaviour, on Ember 3.8

Code:

import EmberRouter from '@ember/routing/router';
import config from './config/environment';
import { set } from '@ember/object';

import { inject as service } from '@ember/service';


const Router = EmberRouter.extend({
  location: config.locationType,
  rootURL: config.rootURL,

  router: service(),
  routeHelpers: service(),

  init() {
    this._super(...arguments);
    this.router.on('routeDidChange', () => {
      // tracking with piwik, if the piwik object has been defined
      if (typeof _paq === 'object') {
        _paq.push(['trackPageView']);
      }

      // tracking with google analytics, if the ga method has been defined
      if (typeof ga === 'function') {
        return ga('send', 'pageview', {
          'page': this.get('url'),
          'title': this.get('url')
        });
      }

      // set current route to the onla accordion, which will open
      // it's corresponding accordion element
      var onlaController = this.routeHelpers.controllerFor('onla');
      set(onlaController, 'currentRouteName', this.router.currentRouteName);

    });
  }


});

Exception:

Exception has occurred: RangeError
RangeError: Maximum call stack size exceeded
    at Registry.isValidFullName (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:16657:20)
    at Registry.has (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:16448:17)
    at Registry.proto.validateInjections (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:16762:25)
    at processInjections (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:15999:28)
    at buildInjections (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:16040:7)
    at injectionsFor (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:16051:12)
    at FactoryManager.create (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:16116:13)
    at instantiateFactory (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:15979:63)
    at lookup (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:15907:12)
    at Container.lookup (https://n204.crealogix.net:4200/assets/vendor-e2a60dfe5e2603e2bdff789c3cdefd1e.js:15751:14)

I wanted to replace 'didTransition' function in my router.js by listening to the 'routeDidChange' event, because I got this deprecation warning:

DEPRECATION: You attempted to override the "didTransition" method which is deprecated. Please inject the router service and listen to the "routeDidChange" event. [deprecation id: deprecate-router-events] See https://emberjs.com/deprecations/v3.x#toc_deprecate-router-events for more details.

[ghost]

@ursqontis for the record, you can listen to the router events from within the router. You should just need to do this.on('routeDidChange'). This snippet has the working example: https://github.com/efx/emberjs-17791/commit/8d8dd5afa1b9841742d1127a95689ab98c085632#diff-f3a289058604b2b069d07bb8e2cda60cR8

[chriskrycho]

If this is intended to be the public API for doing this in EmberRouter, can we document it? cc. @pzuraq @rwjblue @chadhietala? If it's not public API, I'm happy to write a small RFC to make it public API.

[rwjblue]

I think it should generally be considered a bug if service injections fail. The issue in this case is that the router service itself gets router:main injected into it.

What is happening here is:

• an injection so that service:router receives router:main when it is instantiated
• router:main is looked up
• router:main looks up service:router on instantiation (this is done in debug builds for all service injections, so that we can provide a good error when a referenced service isn’t available in the system. As opposed to only erroring if you happen to attempt to use the service)
• in order to instantiate service:router the container attempts to create router:main (to satisfy that injection)
• loop

The fix here is to make the router service not eagerly lookup the router:main...

[chriskrycho]

@rwjblue, how hard a lift is that? I’m going to have a little time in the next few weeks when I could probably hit this if the path is clear!

[rwjblue]

Sorry I missed this at the time @chriskrycho. I think it should be fairly straightforward (e.g. "not too hard") to remove the auto-injection, and only look up the router service when actually pulled on.

[snewcomer]

Hi all! closed by #19405

[sly7-7]

@snewcomer @rwjblue since an other refactoring, the routerService is a 'private' property of the EmberRouter, https://github.com/emberjs/ember.js/blob/master/packages/@ember/-internals/routing/lib/system/router.ts#L164

I wonder if we can just make this prop public (though it still looks kind of weird to me, accessing a service from an object, aiming to wrap some behaviors of this specific object).