-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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 "Notifications is not set" errors. #60473
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lukeelmers
added
bug
Fixes for quality problems that affect the customer experience
review
v8.0.0
Team:AppArch
release_note:skip
Skip the PR/issue when compiling release notes
v7.7.0
labels
Mar 18, 2020
Pinging @elastic/kibana-app-arch (Team:AppArch) |
This was referenced Mar 18, 2020
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
alexwizp
approved these changes
Mar 18, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
lukasolson
approved these changes
Mar 18, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
lukeelmers
added a commit
to lukeelmers/kibana
that referenced
this pull request
Mar 18, 2020
lukeelmers
added a commit
that referenced
this pull request
Mar 18, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
bug
Fixes for quality problems that affect the customer experience
release_note:skip
Skip the PR/issue when compiling release notes
review
v7.7.0
v8.0.0
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Hopefully addresses the issue described in #58429. (I haven't run the cloud tests specified in that issue yet, but I'm fairly confident it addresses the same underlying problem; will run those tests after merging as this is a bug that needs to be fixed either way).
With the usage of the service getter/setter pattern in the
data
plugin, we run into this weird scenario where the setters which are set in the new platformdataPlugin.start()
areundefined
in the legacy world. I suspect this has to do with bundling, causing the legacy world to not share the same instance of those singletons.To users this is most often surfaced in a toast error:
X is not set
, which is thrown when a service a legacy plugin depends on calls, for examplegetNotifications()
.As a result, we need to make sure they are set again in the legacy world. Currently this is happening in
ui/new_platform
.This PR does a couple of things:
plugins/data/public/services.ts
to ensure all of those items are set in the new platform data plugin's setup/start methodsui/new_platform
for the legacy worlddata/public/services
is set inui/new_platform
too. If a new getter is added to the services file, these tests will break ifui/new_platform
has not been updated as well.While this solves the immediate problem and should prevent similar issues from arising in the future, it highlights another reason why we should move away from the service getters pattern toward explicitly passing dependencies down into the services that need them. This is less error prone and more easily testable.
Testing
In the dev console, create a new document:
Then create an index pattern for
foo-*
, with a scripted field that does this:Then create one more document in console:
Navigate to discover and view the
foo-*
index pattern. There should be a shard failure error (expected). Clicking on the error should successfully open the details overlay.Before this fix, you'd get a
Notifications is not set
error instead.