-
Notifications
You must be signed in to change notification settings - Fork 8.6k
[User Storage Service] browser client, React hooks, injection rendering #268312
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
Changes from 35 commits
fee3b2f
d99db24
3705dfd
b2fb0bd
052c487
ed7ddd3
7c49125
1308d1c
de77277
22e8d13
463d71c
90b1ff7
a9ecfb0
c424548
c3798a8
e1b7da5
97de6a9
71a036c
78c8e35
508feb9
dfe0e9a
7303ff7
e4853f7
dea9a24
cccfd08
2307f46
c0b620d
55f07f4
74d5535
6a47424
0470905
06fe6b9
973c116
69371cb
cac10e1
bc92fc1
aa5bad2
5fda572
967884f
c215f23
fcba20b
5d6fa66
5ce9212
bfe43d7
e49e3f9
910eada
bf5946b
0e65c22
bc289b4
0a19ba2
c46bbf9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,6 +99,10 @@ export class InjectedMetadataService { | |
| getFeatureFlags: () => { | ||
| return this.state.featureFlags; | ||
| }, | ||
|
|
||
| getUserStorage: () => { | ||
| return this.state.userStorage; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this should be gated and consumers that need injection of their storage can opt into it. Somewhere in the storage integration, there should be a configuration to opt into injection when the consumer registers their storage type. This should be the case because there could be consumers that have a large payload, where we want to just request the storage item async always.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if we should even inject the user storage information into the HTML that gets served to the user, I think doing so makes it such we can almost never cache the index HTML for an authenticated user on a CDN given it's content is dynamic, without adding this the dimensions for caching heuristics are well known.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I get what you're saying but the pattern of injecting metadata in server-side rendering has been in use for a long time. We already do this with feature flags and UI settings, theme info, etc. It's a useful technique when we need to apply stored values into the hot paths of rendering
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm aware we send along couple of these kind of configurations, however those configurations typically could be scoped to a set of users, but with the user storage addition the eventual generated entry point is now completely unique. This is not a blocker from my side, it's just something I want to confirm that we are sure we'd like to do especially that you mentioned consideration for an async approach.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. I'd like to table this for now. The primary consumer of User Storage will be the project side navigation, using this to store a customized nav for each user / space. I see a risk that would hurt UX if we're not showing the nav to the user asap, because we have to fetch the data in nav plugin setup. I'll call out your point in the epic issue: https://github.com/elastic/kibana-team/issues/2874 |
||
| }, | ||
| }; | ||
| } | ||
| } | ||

Uh oh!
There was an error while loading. Please reload this page.