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

[Fix] Fix bug with missing license information #22485

Merged
merged 1 commit into from
Aug 29, 2018

Conversation

sorenlouv
Copy link
Member

@sorenlouv sorenlouv commented Aug 29, 2018

I thought that license information would always include something like:

{
  features: {
    watcher: { isAvailable: false },
    ml: { isAvailable: false }
  }
}

Turns out we can't rely on this being available, so have to guard the accessors with lodash.

TODO:

  • Add tests

@sorenlouv sorenlouv added the Team:APM All issues that need APM UI Team support label Aug 29, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/apm-ui

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@jasonrhodes jasonrhodes left a comment

Choose a reason for hiding this comment

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

Looks good -- any reason not to do
get(license, 'data.some.path') instead of get(license.data, 'some.path') in these cases?

@sorenlouv
Copy link
Member Author

sorenlouv commented Aug 29, 2018

I try to avoid _.get when a values is guaranteed to be available - and if it isn't available, it points to a bug somewhere else in the system. Using _.get can sometimes hide these bugs.

When I do use _.get I try to do it only for the part that is not guaranteed. In this case I know that license.data is guaranteed to be declared (might be undefined):

https://github.com/elastic/kibana/blob/master/x-pack/plugins/apm/public/store/reactReduxRequest/helpers.js#L14-L17

That's at least the reasoning I tell myself :)

@jasonrhodes
Copy link
Member

@sqren yeah this makes sense with null pointer bugs and all that, I think at some point I changed my mind on this a bit in some cases where I know I want to avoid bubbling that error up to the user in either case. Being able to log something about the unexpected undefined value would be a nice middle ground but I don't know that we have a good way to do that.

Out of curiosity: would an ErrorBoundary have let us show a more useful screen when that value was undefined/blew up? We used them a bit but I was always a little fuzzy on how they work tbh.

@sorenlouv
Copy link
Member Author

I think at some point I changed my mind on this a bit in some cases where I know I want to avoid bubbling that error up to the user in either case.

What I'm hoping is that the error will never make it to the user, since it'll be obvious in dev-mode when everything explodes - but that also sounds like a dangerous way to code, when I put it like that, so I'm on the fence :)

I haven't really used ErrorBoundaries yet but they sound useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:APM All issues that need APM UI Team support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants