-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Consolidate and deprecate Kibana Context #158745
Conversation
b6e50de
to
b59dac9
Compare
b59dac9
to
31bcf35
Compare
Pinging @elastic/appex-sharedux (Team:SharedUX) |
21bd2a1
to
cd5d125
Compare
packages/core/theme/core-theme-browser-internal/src/core_theme_provider.tsx
Outdated
Show resolved
Hide resolved
src/plugins/kibana_utils/public/theme/kibana_theme_provider.tsx
Outdated
Show resolved
Hide resolved
3b77ea6
to
c0ac883
Compare
Co-authored-by: Michael Dokolin <[email protected]>
0439b68
to
117d8c8
Compare
@elastic/kibana-data-discovery Could you weigh in on this test failure? The tests are passing locally, curious if this one is ever flaky? |
I followed up on slack, and to summarize my finding: it seems this test now takes double the time to before those changes. that's why these tests seems to run into timeouts because they now take more than 5s to execute. So it's recommendable to check what causes this performance issue |
@clintandrewhall that spaceship bit cracked me up. I needed that laugh lol |
@@ -76,7 +76,7 @@ module.exports = { | |||
|
|||
// A list of paths to snapshot serializer modules Jest should use for snapshot testing | |||
snapshotSerializers: [ | |||
'<rootDir>/src/plugins/kibana_react/public/util/test_helpers/react_mount_serializer.ts', |
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.
@dmlemeshko do you know anything about the snapshot serializers for jest?
@pheyos what about you Robert, any idea what this is.
I'll start googling now too
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.
interactive setup - LGTM!
18dd4a7
to
50f0d3a
Compare
I've update this PR in light of #159638 -- which demonstrates why we need a unified Context for Kibana. We should be using We need to get #159719 resolved, as well. We're maintaining two code editors, and need to mark the deprecation/export the packaged code editor as soon as possible. |
💔 Build FailedFailed CI Steps
Test Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
Unreferenced deprecated APIs
History
To update your PR or re-run it, just comment with: |
I'm going to have to close this PR and start a new one... there's been way too much churn, and I need to adjust the strategy to a Root context, rather than a portable one. Thanks for the help, all. |
Background
There are a number of context-related issues that have existed for a while:
Kibana Context Provider "spaceships"
Kibana uses React Context to inject stateful dependencies from its plugins. The number of Context Providers a plugin can end up using can create a "spaceship" of descendents. So we've needed a Provider "composer"-- a quick way of combining Providers into a single React node-- for a while now.
Untyped contextual services when using
KibanaContextProvider
anduseKibana
hookThe
KibanaContextProvider
allows for untyped "storage" of a plugins dependencies. The hook, therefore, relies on regularly inconsistent usage, which creates runtime errors that cannot be caught in development.Duplication of Theme and other core providers
The
kibana_react
plugin has been providing both atheme
and untyped "services" provider, these utility Providers have been copied around quite a bit... and sometimes enhanced. We've needed to move these to a package for a while now.Inability to set global context parameters consistently.
We've wanted to fix "global default" issues like #56406 for a while now, but we haven't had a universal context from which every plugin descends. By creating a single consumable provider, we can propagate
uiSettings
and other defaults consistently.Inconsistent "dark mode" and other context issues
While plugins fairly consistently wrap their application renders with
i18n
andEuiProvider
(or similar), async or lazy loads are inconsistent. As a result we get "light mode" renders in "dark mode" contexts. A base context that can be easily consumed in these async renders is needed.Summary
This PR does a number of things:
theme
Providers into a single package-based Provider, (removing circular dependencies).Creates a Provider composition utility for combining an array of Providers and enforcing a union of requiredprop
types.Uses this utility toCombineI18nProvider
and the newKibanaThemeProvider
to create a baseKibanaContextProvider
.Pre-commit next steps
Post-commit next steps
createKibanaReactContext
anduseKibana
.