Changes in scope hierarchy cause errors (NGI.InspectorAgent.inspectScope is not a function)

[tdhsmith]

I'm getting a TypeError: NGI.InspectorAgent.inspectScope is not a function in my current app, triggered at Scope.js line 62. After some initial digging, I think the issue might be a circular dependency here:

bootstrap.js --> App.js --> InspectorAgent.js --> Scope.js
     ^                             ^                  |
     |                             |                  |
   start                           *------------------*

AFAICT, NGI.InspectorAgent is an empty object within Scope.js, which seems to match with the dependency graph, but I'm also not very knowledgable in advanced module stuff, so I could be jumping to conclusions.

Haven't figured out the exact trigger or isolated it yet, but I think it's related to automatic unwrapping of $resource requests while the inspector is looking at them. If the above guess is correct though, it might be triggering on any cases where that watcher sees new children in an open scope?

Will try to come back to this in the next couple days with a minimal demo.

[tdhsmith]

Ok so I've built a minimal test case:

http://run.plnkr.co/plunks/CBpoKcZUnU7hUlvkVGW1/

The issue happens whenever the list of a scope's children changes. Though error on line 62 aborts the function before the child list can be updated, so some changes won't even trigger the error. (e.g. in the sample, the error doesn't fire when removing the child: the parent's oldChildIds is still [] so it doesn't notice the difference).

So really the surface issue here is that changes in the scope hierarchy are not reflected in the inspector when the pane is open.

I'm pretty confident now that the cause is the circular dependency shown above. Do you guys have a strong opinion on the best direction forward? You could delay the require('./InspectorAgent') until the first time the Scope constructor runs or something, but that's not a super clean. Or you could try to pass instances around and/or decouple the modules a bit, but those require a lot more inspection and planning... (Maybe the InspectorAgent's traversal functions could take a scopeProvider parameter instead of calling NGI.Scope directly?)

[DrewML]

Nooooooo, circular dependency issue!

Thanks for the report @tdhsmith. I thought I had broken all the circular deps when migrating the extension to browserify, but apparently I missed one (Madge confirms this).

┌─[andrew] [master → origin ✓] [~/Documents/ng-inspector/src/js]
└─▪ madge --circular .
InspectorAgent -> Scope

In regards to the solution, my preference would be to decouple the modules a bit more to the point that whatever functionality is creating the circular dep be broken out to a 3rd module. We are trying to plan out some bigger architectural changes to the extension right now that would likely help with that, but I suspect those are going take quite a while.

It would probably be a good idea if we patch this in the meantime. I can't give an explicit suggestion for a fix atm because I simply haven't spent enough time looking at that part of the codebase recently, but I think we're very open to suggestion if you want to take a whack at it.

[DrewML]

In the interest of avoiding errors for end-users, I think we should go with a super ugly, but effective patch for the time being. I've been using the extension a bit tonight working on some other stuff and the errors are pretty frustrating.

module.exports = InspectorAgent;

var NGI = {
    Scope: require('./Scope')
};

Like you mentioned @tdhsmith, we'll just delay the require for now until InspectorAgent itself has finished exporting. We should just make sure to add a test to prevent an accidental regression in the future. This should buy us some time until we make some bigger changes to how components are structured internally within the extension.