unmount() reloads browser while trying to get $uiRouter instance

[dassennato]

Hi,

our project is using ui-router@1.0.0-beta.1 and seems like since is not a final 1.0.0 release, it does not have a provider for '$uiRouter' nor a dispose() method.
I believe because of that, when the unmount() lifecycle function executes the following:

function unmount(opts, mountedInstances, props = {}) {
  return new Promise((resolve, reject) => {
    // https://github.com/single-spa/single-spa-angularjs/issues/53
    const uiRouter = mountedInstances.instance.get("$uiRouter");
    if (uiRouter) {
      uiRouter.dispose();
    }

my microfrontend breaks throwing this error:

zone.min.js:13 Uncaught Error: [$injector:unpr] Unknown provider: $uiRouterProvider <- $uiRouter
http://errors.angularjs.org/1.5.9/$injector/unpr?p0=%24uiRouterProvider%20%3C-%20%24uiRouter

and then the browser gets reloaded.

I've looked at that source code of ui-router@1.0.0-beta.1 and it seems like '$uiRouter' is provided as 'ng1UIRouter', so if I patch the unmount() method to:

var uiRouter = mountedInstances.instance.get("ng1UIRouter");

it finds the instance but it still breaks since it does not have the dispose() method.

I'm not sure what's the dispose() equivalent method of 'ng1UIRouter', if exists, or what's it for, but if I simply remove all those lines it works perfect for me, I don't see any sign of #53 which is the issue that those lines are meant to fix.

Wrapping up, I wonder if it'd be desired to have a flag to toggle that feature, I know that I'm using a beta version of ui-router and should consider upgrading it, we've tried but got a lot of issues making us to give up. Anyways, I don't see a conditional either, which should check if opts.uiRouter is truthy, based on that I believe this issue may happen as well if your app doesn't have uiRouter at all.

[filoxo]

I looked into this a little bit though I'm not as familiar with Angularjs. I believe this is due to using that old of a version of ui-router. router.dispose() was added in ui-router-core@3.1.0 and that was updated in angular-ui-router@1.0.0-rc.1 released on 2017-01-09. 1.0.0-beta.1 was released on 2016-06-30 so it definitely predates that upgrade. Is it possible for you to update to at least 1.0.0-rc.1? I think we could check for that method before calling it but I imagine you'd encounter the same bugs as noted in that issue you linked to so upgrading would likely be less problematic for you in the long run.

[dassennato]

I looked into this a little bit though I'm not as familiar with Angularjs. I believe this is due to using that old of a version of ui-router. router.dispose() was added in ui-router-core@3.1.0 and that was updated in angular-ui-router@1.0.0-rc.1 released on 2017-01-09. 1.0.0-beta.1 was released on 2016-06-30 so it definitely predates that upgrade. Is it possible for you to update to at least 1.0.0-rc.1? I think we could check for that method before calling it but I imagine you'd encounter the same bugs as noted in that issue you linked to so upgrading would likely be less problematic for you in the long run.

I will try to upgrade to rc-1 then, thanks for the tip @filoxo, but either way, if someone has the microfrontend set up like this:

singleSpaAngularJS({
    angular,
    mainAngularModule: app,
    uiRouter: false,
    ...
});

shouldn't have to be there a conditional like this?

      if (opts.uiRouter) {
        var uiRouter = mountedInstances.instance.get("$uiRouter");

        if (uiRouter) {
          uiRouter.dispose();
        }
     }

[filoxo]

Hmm possibly? The docs are unclear on what .get would return if that module has not been instantiated before (a new instance? or something falsy if there are no previously created instances?), and I'm still very rusty. But I still did a little digging and found the source and it does appear to try to instantiate if the module if not already:

https://github.com/angular/angular.js/blob/9bff2ce8fb170d7a33d3ad551922d7e23e9f82fc/src/auto/injector.js#L876-L898

Can you confirm that that's the case? Perhaps .has would be a better check? Otherwise yes it could just skip that block if opts.uiRouter is falsy.

if (mountedInstances.instance.has("$uiRouter")) {
  var uiRouter = mountedInstances.instance.get("$uiRouter");
  uiRouter.dispose()
}

I'm also thinking that this could be improved by checking that the router module has a dispose method before calling it. And could even warn the user if their router could not be disposed of.

if (mountedInstances.instance.has("$uiRouter")) {
  var uiRouter = mountedInstances.instance.get("$uiRouter");
  if(!uiRouter.dispose) { 
    console.warn('The uiRouter instance does not have a dispose method and so it cannot be cleaned up.')
  } else {
    uiRouter.dispose();
  }
}

What do you think?

[joeldenning]

@filoxo I think your proposal above is good. Do you have interest in sending in a PR with the fix? If not, I may have time to do so.

[filoxo]

I won't get to this so feel free to implement it.

[joeldenning]

Fix released in https://github.com/single-spa/single-spa-angularjs/releases/tag/v4.2.2