Router.map callbacks hung off Router constructor

[jasonmit]

Router.map hangs the callback functions it receives within Router.constructor.dslCallbacks. In a traditional client-side rendered Ember-world, this is harmless.

However, when you take into account FastBoot, this becomes unsafe if you call Router.map multiple times within your application those functions can leak across instances. A particular use-case is we registered routes dynamically depending on user state and we do this inside of an instance-initializer.

This isn't a terribly uncommon pattern; example is someone who wants to register routes in the language of the user. This is likely the place where that would happen.

Where this becomes a problem in FastBoot is each time a request is made the callback is pushed to a property on the constructor and then enumerated over and invoked. So for each request, a new callback is pushed into the array of callbacks and no clean up takes place (a memory leak and unsafe where functions from previous app instances are invoked).

https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/router.js#L89-L96

Are the lines in question and so I was wondering if anyone would be opened to me moving these callbacks elsewhere, such as the application instance. This will have larger ramifications, so would like to gather interest or feedback first.

[nathanhammond]

Yep, definitely an issue. This is made more frustrating by the many layers everything here has to go through in order to function the way Ember would like. If you can come up with a quick patch I would be happy to review it and we would likely land it. Depending upon the complexity/invasiveness the core team will decide what branch (canary, beta, release) to target. Probably a late addition to the 2.8.0-beta.X series of which the first one is planned for next week.

If it becomes too ridiculous the RouterDSL is on my list of things to rework after finishing route-recognizer.

[jasonmit]

@nathanhammond right now I don't have a good idea on how I could patch it while keeping the API intact. I was thinking of perhaps exposing a new API for registering callbacks that are concatted with constructor.dslCallbacks during _initRouterJs.

Let me know if you have any ideas and feel free to ping me on Slack.

[nathanhammond]

Everything in that area should be considered private API. I don't believe we need to maintain API consistency in addressing this bug. (Somebody else please chime in and correct me if I'm wrong.)

The only addon I know of which really abuses this portion of the router is one of mine, ember-route-alias, and even I don't touch it.

[jasonmit]

I meant to say I don't know of a way of resolving this without implementing a new API for registering routes designed to be used at runtime instead of going through the constructor method. I can keep the existing API intact but the new way might look similar to exporting a function from router.js.

[nathanhammond]

You could change the way we save off those references such that they're captured in a closure that is only accessible during that run? I think that should be possible.

[jasonmit]

We spoke offline, the outcome was to spike two possible solutions and discuss.

[jasonmit]

/cc @wycats

Based on our discussion today. Feel free to add your input since you floated a few ideas, the core of the issue being the need for Router.map's callback to handle conditional logic and a similar concept of needs.

[jasonmit]

This is what I've ended up with to remove the need of calling Router.map inside of an instance initializer.

https://github.com/jasonmit/ember-routermap-inject-service

It's obviously touching private APIs, and I'd be happy to land something like this in the API if others feel it might be useful.

Another option might be exposing a service that enables registering routes at app runtime safely. This would end up replacing Router.map.

// app/services/router.js
export default EmberRouter.extend({
  session: inject.service().
  map() {
    let session = this.get('session');

    if (session.get('isAdmin')) {
      this.route('admin', { path: '/' });
    } else {
      this.route('home', { path: '/' });
    }

    this.route('about', { path: '/' + session.translate('urls.about') });
    this.route('contact', { path: '/' + session.translate('urls.contact') });
  }
});

[nathanhammond]

So what you've done is make sure that each invocation is identical, allowing Ember to continue to hang its callback functions off of Router.constructor.dslCallbacks. I feel like this is a good workaround, but doesn't solve the underlying problem.

I feel like we should go ahead and fix this at the fundamental level of making sure we clean up after ourselves instead of working around it. @jasonmit did you attempt to head down that path as well before going with the addition to the RouterDSL prototype?

[jasonmit]

@nathanhammond no it certainly doesn't fix the issue at the core, it does though eliminate having to invoke Router.map with the app instance in scope though. I did chat with @wycats and what he said was basically the ability to needs avoids surfacing this bug.

That said, the way I would fix this at the core boils down to never hanging the callback off the constructor for later access. Instead, just resolve and invoke a module which exports a function, the Router.map callback, per-instance of the app that is booted. If this approach is fine with everyone, I'd be happy to send a PR.

[nathanhammond]

@jasonmit I'd like to see that!

[rwjblue]

FWIW, we use that same pattern for engines. ember-engines uses addon/routes.js that just exports the callback.

[jasonmit]

@rwjblue noticed this while researching the bug. I'll be sure to piggyback off that work versus reinventing the wheel.

[pixelhandler]

@jasonmit @rwjblue @nathanhammond is this still an issue, perhaps we should close; what do you think?

[jasonmit]

@pixelhandler issue is still active

[wycats]

@tomdale I agree that this is still an active issue, and relevant to FastBoot. Any thoughts?

[rwjblue]

We need to add an assertion when Router.map is called after an ApplicationInstance instance is created.