Bypass unresolved route promises in route-debug

[steventsao]

Description

This PR skips rendering a route when it is wrapped in an Engine that is not resolved. Since the route tree refreshes on route update, those unresolved engines will eventually be included in the pane, as shown in the screenshot.

Another WIP adds Ember Engine to the inspector, but is incomplete and is nontrivial because the inspector has to know the implementation detail of an Engine.

My PR does not add the concept of an Engine but skips all unresolved Promises. This reenables the tab to show the routes in the right hierarchy and fresh data when the Promises are resolved. Let me know if you can consider merging this to unblock other users, ie #1173 since April.

Screenshots

Thanks!

Closes #1173

[RobbieTheWagner]

@pzuraq @chancancode does this sound like a good fix or should we instead be waiting on the promises here?

[steventsao]

In my observation, buildSubtree keeps getting called until all the promises are settled anyway, so skipping or waiting for the engines in the initial calls go by faster than I can perceive the difference. I'm interested in your thoughts tho.

[RobbieTheWagner]

@pzuraq @chancancode does this seem like a good fix for now or what are your thoughts?

[steventsao]

I spoke to @pzuraq, who is "not very familiar with the inspector," and so is not comfortable reviewing this.

Do you have bandwidth @chancancode ?

FYI @rwwagner90

[RobbieTheWagner]

@villander perhaps you have some insights here regarding engines?

[steventsao]

Found a bug in changing engines. Closing until I need a review @rwwagner90

[RobbieTheWagner]

@steventsao was the bug you mentioned fixed?

[steventsao]

@cyril-sf Can you let me know what visual change you were going with your PR? Were you planning on adding an Engine type in a container that would show inside the route tab, or would routes inside an engine render the same way as those not contained in one?

Asking so I can either build on top of it or separately but with consistency as what you were going for. Thanks.

[rwjblue]

Generally speaking, this change is fine (and prevents the specific error that folks have reported). I do think that we should do something more than continue though.

---

If you see here, buildSubTree is invoked from a computed property that only depends on router:

ember-inspector/ember_debug/route-debug.js

Lines 60 to 73 in d4f1fbb

	routeTree: computed('router', function () {
	const router = this.router;
	const routerLib = router._routerMicrolib || router.router;
	let routeNames = routerLib.recognizer.names;
	let routeTree = {};
	for (let routeName in routeNames) {
	if (!hasOwnProperty.call(routeNames, routeName)) {
	continue;
	}
	let route = routeNames[routeName];
	buildSubTree.call(this, routeTree, route);
	}
	return arrayizeChildren({ children: routeTree });
	}),

However, since router is also a CP that has no dependent keys that will force a re-computation (the namespace.owner dep key won't have an effect on re-running as routes are resolved) buildSubTree is not guaranteed to be called again after the promise has resolved:

ember-inspector/ember_debug/route-debug.js

Lines 23 to 25 in d4f1fbb

	router: computed('namespace.owner', function () {
	return this.get('namespace.owner').lookup('router:main');
	}),

I've left an inline suggestion that should ensure that buildSubTree is invoked again for this specific route.

[rwjblue]

buildSubtree keeps getting called until all the promises are settled anyway

buildSubTree is definitely called many times (once for each route),

I think we should do something like:

      // Skip when route is an unresolved promise
      if (typeof routeHandler?.then === 'function') {
        // ensure we rebuild the route tree when this route is resolved
        routeHandler.then(() => this.notifyPropertyChange('routeTree'));
        controllerName = '(unresolved)';
        controllerClassName = '(unresolved)';
        templateName = '(unresolved)';
      } else {
        controllerName =
          routeHandler.get('controllerName') || routeHandler.get('routeName');
        controllerFactory = owner.factoryFor
          ? owner.factoryFor(`controller:${controllerName}`)
          : owner._lookupFactory(`controller:${controllerName}`);
        controllerClassName = this.getClassName(controllerName, 'controller');
        templateName = this.getClassName(handler, 'template');
      }

      subTree[handler] = {
        value: {
          name: handler,
          routeHandler: {
            className: routeClassName,
            name: handler,
          },
          controller: {
            className: controllerClassName,
            name: controllerName,
            exists: !!controllerFactory,
          },
          template: {
            name: templateName,
          },
        },
      };

This ensures that the subtree structure itself is correct (and does indicate that something is at this location / route name), it also ensures that a re-computation will happen when the promise resolves.

[RobbieTheWagner]

Thanks for this detailed suggestion @rwjblue! @steventsao would you be interested in updating the PR accordingly?

[steventsao]

Done @rwwagner90

[rwjblue]

Thank you!

[steventsao]

Awesome, looking forward to the next release!

cc @rwwagner90