Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[kbnUrl] ensure that routes don't load after change #11246

Closed
wants to merge 2 commits into from

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Apr 14, 2017

When all of the promises in a route's resolve functions resolve immediately the router will render the view in the same tick of the digest loop. This is ideal for performance but can lead to view flickering and unnecessary controller loading if kbnUrl.change() (or similar) is called within the resolve functions (or setup work).

This is caused by the fact that calling $location.url() (or similar) does not trigger a route change until the next tick of the digest loop (for good reason). We have historically gotten lucky because most of our routes load values from the server, but this is not the case for all pages and isn't true for the vis/dashboard landing pages.

To trigger the issue:

  • Delete your index patterns
  • Visit kibana and quickly click the visualize nav item
  • See the outlines of the vis landing page render before the management app is rendered again

To fix this we need to ensure that route resolve() functions do not resolve before the $location service syncs with the $browser service, which is what triggers the router to load a new url.

To do this the kbnUrl service will now:

  • create a urlChangedPromise for each call to $location.url() that only resolves once $location.absUrl() and $browser.url() return the same value
  • keep an array of all pending urlChangedPromises
  • after any setup work completes or fails, call kbnUrl.awaitPendingUrlChanges() and delay resolution until that promise resolves.

When route setup work does not call kbnUrl methods, kbnUrl.awaitPendingUrlChanges() returns a resolved promise so that route load time will not increase and will simply fill the gaps caused by the specific timing required to reproduce this error.

@spalger spalger added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Apr 14, 2017
@cjcenizal
Copy link
Contributor

I have a couple questions:

  1. Is there a solution to this problem which is more idiomatic to how angular-router works? E.g. http://stackoverflow.com/questions/18256106/cancel-route-and-redirect-to-other-route-without-displaying-the-original-content

  2. What's the role of kbnUrl in relation to $location.url? Are there times we should use one but not the other? If so, can we write some general instructions for use in kbnUrl?

@spalger
Copy link
Contributor Author

spalger commented Apr 15, 2017

Is there a solution to this problem which is more idiomatic to how angular-router works

The solution to that stack overflow question assumes that the route resolve functions will end up returning rejected promises. This is problematic for two reasons:

  • kbnUrl.change() doesn't/shouldn't throw an error
  • route errors that are not caught and handled trigger the fatal error page, a good default i think

This means that we would still need to find a way to change the current route/location and prevent the router from rendering the old route until the change went into effect (sometime during the next digest cycle).

What's the role of kbnUrl in relation to $location.url?

I added a comment to url.js to describing how kbnUrl is intended to be used for all URL manipulation in place of $location

@cjcenizal
Copy link
Contributor

cjcenizal commented Apr 17, 2017

I added a resolver to Visualize, and verified that the template won't load until the promise has resolved:

uiRoutes
.defaults(/visualize/, {
  requireDefaultIndex: true
})
.when(VisualizeConstants.LANDING_PAGE_PATH, {
  template: visualizeListingTemplate,
  resolve: {
    loadAsyncDependency: function(kbnUrl, $q) {
      var deferred = $q.defer();
      setTimeout(() => {
        deferred.resolve();
      }, 3000);
      return deferred.promise;
    }
  },
  controller: VisualizeListingController,
  controllerAs: 'listingController',
});

I then verified that if we reject the promise and use kbnUrl to redirect, the Visualize landing page still doesn't render, so there's no flicker:

  resolve: {
    loadAsyncDependency: function(kbnUrl, $q) {
      var deferred = $q.defer();
      setTimeout(() => {
        deferred.reject();
        kbnUrl.change('/discover');
      }, 3000);
      return deferred.promise;
    }
  },

What's stopping us from fetching the default index pattern and then rejecting the promise and redirecting like this, if there is no default index pattern?

And for context, I'm concerned about the complexity this PR adds. I'm also afraid it may only be addressing the symptoms of an underlying problem. I haven't done a thorough investigation, but I feel like there's a significant amount of complexity in the way we're implicitly configuring routes to load the default index pattern and redirect to management when there is none. If the above pattern solves our problem, then I think we can significantly simplify the code and improve its maintainability by refactoring this system so that each app explicitly defines its dependency upon a default index pattern and then redirects to Management.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v5.4.0 v5.5.0 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants