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

If a value is deleted from the nav link collection, invalidate in-order cache #7227

Merged

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented May 18, 2016

The UiNavLinkCollection class maintains a list of nav links shown in the Kibana UI. Instances of this class have a property called inOrderCache that is meant to contain the list of nav links, in order of their order properties. This inOrderCache property is populated when the inOrder() instance method is called the first time; subsequent calls to the inOrder() method simply return the list in inOrderCache.

As the UiNavLinkCollection inherits from the Collection class, it also has a delete() instance method that can be called to delete a nav link from the list of nav links. This delete() method needs to be overriden to invalidate the inOrderCache each time a link is deleted. Otherwise, the next time inOrder() is called, the stale cached nav link collection is returned, with the deleted object still in it. This PR implements this fix.

Otherwise, the next time inOrder is called, the stale nav link collection that is
in the cache is returned, with the deleted object still in it.
@@ -26,4 +26,9 @@ export default class UiNavLinkCollection extends Collection {
return this[inOrderCache];
}

delete(value) {
super.delete(value);
this[inOrderCache] = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about moving this before super.delete() and returning the calls return value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, makes sense as delete returns a boolean. Thanks!

@spalger
Copy link
Contributor

spalger commented May 18, 2016

LGTM

@ycombinator ycombinator merged commit e2754b3 into elastic:master May 18, 2016
@ycombinator ycombinator deleted the invalidate-navlink-cache-on-delete branch May 18, 2016 02:09
@epixa
Copy link
Contributor

epixa commented May 23, 2016

I'm singling this PR out purely for convenience sake: Especially for PRs that don't have an associated issue, it'd be really helpful to provide a description about what problem is being solved.

@ycombinator
Copy link
Contributor Author

Makes sense. I'll update the description now.

@ycombinator
Copy link
Contributor Author

@epixa Description updated.

@epixa
Copy link
Contributor

epixa commented May 23, 2016

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants