-
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
Cleanup spaces plugin #91976
Merged
Merged
Cleanup spaces plugin #91976
Changes from 7 commits
Commits
Show all changes
27 commits
Select commit
Hold shift + click to select a range
fad2f42
Convert relative imports to absolute imports
jportner 388206d
Convert more relative imports to absolute imports
jportner dc49b70
Temporarily remove `import type` directive
jportner 25a9066
Sort imports
jportner 97cc9bb
Apply `import type` directive
jportner 4ad077f
Add lazy loading of reusable Spaces UI components
jportner f022c4c
Reduce bundle size
jportner 59a921c
Fix jest test error
jportner 6284843
Refactor CTS flyout
jportner 0872427
Move summarizeCopyResult
jportner 9341c0c
Split CTS flyout into a separate chunk
jportner 0080228
Lazy load NavControlPopover
jportner f3126fd
Refactor SpaceAvatar
jportner 9b6cf54
Lazy load SpaceAvatar
jportner c922c54
Remove dependency on @kbn/std package
jportner b8d93b0
Change CTS flyout to use SpacesContext
jportner 6242d1a
Merge branch 'master' into pr/jportner/91976
jportner a21ca9a
Abstract out lazy-loading from reusable components
jportner 032b5cb
Use error boundaries when lazy-loading components
jportner bebf3b2
Rename `getSpacesContext` to `getSpacesContextProvider`
jportner b792353
Fix type check for unit test
jportner 181ddcb
Fix i18n error
jportner 2612620
Remove unused parameter
jportner 1b6554b
Merge branch 'master' into pr/jportner/91976
jportner 1b6c5e5
PR review feedback
jportner 38fb012
Fix jest unit tests
jportner 99ab44c
Merge branch 'master' into cleanup-spaces-plugin
kibanamachine File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Oops, something went wrong.
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.
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.
I probably don't have enough context (lol, see what I did there?) so might be better to catch up tomorrow but it would be nice to be a bit clearer here in regards to naming.
getSpacesContext
to me sounds like we're getting a React Context object for spaces. That's usually a static object which contains a Provider and a Consumer component. In this case however, we are wrapping it inReact.lazy
to create a lazy loaded component so it's just the Provider component by the looks of it? In that case it might be clearer to call the methodgetSpacesProvider
and the return valueSpacesProvider
.Having said that, Context (returned by
createContext
) is usually a static object in React so we could just make that available through the spaces plugin contract directly rather than having these getter functions and having to memoize them.Anyways, let's catch up tomorrow, just adding this as a reminder.
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.
Yea, I share the same concerns. React context providers are usually static, and the value they provide mutates. I'm not sure to understand why we need an lazy accessor around a context provider?
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.
I'm open to naming changes!
Sort of. It's a "wrapper" component that includes the context provider. We need this because when this wrapper is mounted, it makes a series of API calls and caches the results, then makes those available to its children via the context provider.
The function to obtain this wrapper within the Spaces plugin is called
getSpacesContextWrapper
, but the one that is exposed to consumers in the public contract is calledgetSpacesContext
. We could rename the latter to something likegetSpacesContextProviderWrapper
but I don't know what value that really adds for consumers, it's an implementation detail IMO. They don't need to consume the context provider, they just need to know that they have to render the flyout, space list, etc. as a descendant of this wrapper.I don't think so, we need the wrapper.
Because it's not just a context provider, it's the wrapper that also fetches data. Also, to flip that question around: why not provide a lazy accessor? All of the other components have lazy accessors. It seems this approach is more consistent.
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.
Caught up with Joe and I can't think of a good alternative to avoid the architectural complexity with the approach here.
To fulfil the brief, we need a way to make the spaces state available globally, so that we can provide drop-in components for developers that share that state. However, we're not able to render the context provider at the root level of the application ourselves (since spaces is a plugin) and we can't provide static exports (due to the restriction imposed by the platform), hence the indirection using dynamically created and memoized context providers, that require developers to consume them using the spaces plugin contract rather than static imports like EUI.
I'm guessing this is a bit of an edge case and other plugin developers don't have the same requirements to share core functionality like spaces does. However, I would argue that certain things like i18n, routing, session information, authenticated user information and in this particular case spaces are global concerns that should be available throughout the application. Usually these are managed at the root level in React applications so that any child component has access via hooks and context but this isn't really possible with the way we've currently modelled spaces as a plugin.
Happy to leave this as is for now but I am slightly worried about the complexity and maintainability of the current solution.