-
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
[Embeddables rebuild] Add new registry #176018
[Embeddables rebuild] Add new registry #176018
Conversation
/ci |
Pinging @elastic/kibana-presentation (Team:Presentation) |
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.
Still working through the PR but wanted to push some of the comments.
const content = useStateFromPublishingSubject(contentSubject); | ||
const viewMode = useInheritedViewMode(thisApi) ?? 'view'; | ||
|
||
return viewMode === 'edit' ? ( |
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.
How about lazy loading component. Examples should show best practices and best practices recommend keeping bundle size as small as possible.
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.
That's a good point. This markdown example is mostly there for testing purposes at the moment. I've created this follow-up issue to describe further examples that should be built, and an async-imported embeddable is high up on the list.
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
useImperativeHandle(ref, () => thisApi, [uuid]); | ||
|
||
return { thisApi, parentApi }; |
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.
Why does this return parentApi
when parentApi is available at thisApi.parentApi
. How about just returning thisApi
?
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.
Ah, not sure why I did that. Removed the extra return in 1f94fd4
key: string, | ||
factory: ReactEmbeddableFactory<StateType, APIType> | ||
) => { | ||
registry[key] = factory; |
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.
This should throw if key already exists in registry to prevent plugins from overwriting factories.
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.
Done in 1f94fd4
latestChanges[key] = latestState[key]; | ||
} | ||
} | ||
if (Object.keys(latestChanges).length > 0) { |
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.
How about just
return Object.keys(latestChanges).length > 0
? latestChanges
: undefined;
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.
Good call, done in 1f94fd4
currentState?: Partial<StateType> | ||
) => boolean; | ||
|
||
export type EmbeddableComparatorDefinition<StateType, KeyType extends keyof StateType> = [ |
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.
Why is the definition stored in an array vs an object? Accessing parts of the definition is cryptic with values like comparators[key]?.[2]
instead of accessing a named key like comparators[key]?.comparatorFunction
. I would recommend converting this type to an object with named keys unless there is a good reason for an array.
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.
Yes, the cryptic way to access the elements is not ideal.
This is somewhat mitigated by the fact that it's a tuple, not an array, meaning that Typescript can correctly infer the different types of each element.
The reason it's implemented this way is a tradeoff of more complicated definition code for the sake of less verbose adoption code. To illustrate, compare the example below:
Object
const { unsavedChanges, resetUnsavedChanges } = useReactEmbeddableUnsavedChanges(
uuid,
markdownEmbeddableFactory,
{
firstName: {
subject: firstName$,
setter: setFirstName,
},
lastName: {
subject: lastName$,
setter: setLastName,
},
profession: {
subject: profession$,
setter: setProfession,
},
age: {
subject: age$,
setter: setAge,
comparatorFunction: compareAges,
},
otherKey: {
subject: otherKey$,
setter: setOtherKey,
},
}
);
Tuple
const { unsavedChanges, resetUnsavedChanges } = useReactEmbeddableUnsavedChanges(
uuid,
markdownEmbeddableFactory,
{
firstName: [firstName$, setFirstName],
lastName: [lastName$, setLastName],
profession: [profession$, setProfession],
age: [age$, setAge, compareAges],
otherKey: [otherKey$, setOtherKey],
}
);
Consider that this is meant to be used as a hook inside an already fairly complicated component, that we anticipate some Embeddables to have a lot of state keys, and that the tuple setup is very similar to how other react hooks work.
I've attempted to remove some of the confusion in 1f94fd4 by adding names like const subject = comparator[0]
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.
These changes are really clean and show the benefits of the new embeddable system. I really like how the state diffing is getting pushed down to the embeddable. This should fix a lot of the unsaved changes edge cases.
export const apiPublishesUnsavedChanges = (api: unknown): api is PublishesUnsavedChanges => { | ||
return Boolean( | ||
api && | ||
(api as PublishesUnsavedChanges).unsavedChanges && |
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.
Should this also check that typeof is function? apiPublishesWritablePanelTitle tests for typeof function. Just wondering if this should be consistent that other interface guards
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 haven't been very consistent with these typeof
checks to be honest. Mostly so far, I expect the name being present will be enough info for a type guard, but of course there is always the risk that someone makes an API with the exact same key of a different type, which will pass this type guard and end up throwing a runtime error.
In short, I'm not sure how strict to be with these, which is why the inconsistency is there. What do you think?
Either way, I'd expect us to change all of these to be consistent in a followup.
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 think its ok as is. We should decide one or the other and then make sure all interfaces are consistent. I would vote to check for function to provide a better guard
import { | ||
ReactEmbeddableRenderer, | ||
EmbeddablePanel, | ||
reactEmbeddableRegistryHasKey, |
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.
How come reactEmbeddableRegistryHasKey is statically imported vs exposed via a plugin contract? Will this have bundle size penalties? Are there state management concerns?
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.
This is a great question!
I'm trying a new strategy here where we keep a registry directly at the module level of a plugin. All plugin code is stateful, so we will end up with just one instance of any code placed in the module.
When exported statically like this, it allows consumers to register their factories anywhere simply by importing and running the function. No prop drilling, fancy contexts, or plugin start contract juggling necessary.
I don't think this has any implications on bundle size, as the code is tiny. As far as I can tell there is the same amount of timing / state management risk using this strategy, as there is using a registry class on the plugin start contract. I.e. a consumer could theoretically register their embeddable factory after a 10000ms delay in both cases.
If we do run into problems down the line, we could fairly painlessly transition to a registry on the plugin contract instead.
@@ -71,6 +71,7 @@ export class PanelNotificationsAction implements ActionDefinition<EnhancedEmbedd | |||
}; | |||
|
|||
public readonly isCompatible = async ({ embeddable }: EnhancedEmbeddableContext) => { | |||
if (!embeddable?.getInput) return false; |
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.
This is just a stop gap until this action has been converted? If you had react embeddable then panel notification action would not work until this is converted- right?
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.
Yes exactly, these are just to prevent errors when using any React Embeddable.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Unknown metric groupsAPI count
async chunk count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
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—code review only
@@ -24,6 +24,7 @@ interface Context { | |||
} | |||
|
|||
export async function isEditActionCompatible(embeddable: IEmbeddable) { | |||
if (!embeddable?.getInput) return false; |
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.
If this isn't just temporary, it would be nice to see a comment. If you're going to change this anyway, disregard.
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.
This is just temporary until #175138 is completed. It's there just to avoid a runtime error when a React Embeddable is on the Dashboard.
Creates a new registry for `React Embeddables` and allows the Dashboard to render them.
Creates a new registry for `React Embeddables` and allows the Dashboard to render them.
Creates a new registry for `React Embeddables` and allows the Dashboard to render them.
Summary
This PR is the next major piece of #167429. It closes #167892 and #167893 by building a new registry for
React Embeddables
and allowing the Dashboard to render them.This is the framework that all existing embeddables will be moved onto, and replaces the
serialization / deserialization
capabilities of the old Embeddable framework. For more information, see the architecture document.Once this PR merges, the new Embeddable system will be complete.
React Embeddables
Note
The new Embeddable framework is temporarily named
React Embeddables
to stress their closer coupling with React. It will be renamed toEmbeddables
once the legacy Embeddables framework is removed.The React Embeddables framework comes with a number of new systems, which when composed, allows engineers to register serializable components that interact with a container or page. These subsystems are:
The Registry
React Embeddable factories are stored in a registry at the module level of the Embeddable plugin. This means that you can put
embeddable
in yourrequiredBundles
, import the registration function and call it from anywhere.The Factory
The React Embeddable factory is the replacement for the
EmbeddableFactory
class defined insrc/plugins/embeddable/public/lib/embeddables/embeddable_factory.ts
. The new factory is much simpler, taking the form of an object with two methods:injectReferences
, andmigrate
and also must cast the final results into the state type of the factory.Parenting
This PR also replaces the parenting system from the legacy Embeddable system. Parenting is now accomplished with a simple React Context.
ReactEmbeddableParentContext
which must provide aPresentaitonContainer
.When an Embeddable is rendered, specifically during the
useReactEmbeddableApiHandle
call, it finds its parent via the context and registers itself with the parent. The hook then returns the API it creates, and the parent API.Instead of parents passing children all of their inherited states, the children get access to the full API of their parent, and from there, they can select, use, or inherit state from their parent as necessary.
Unsaved Changes
This PR also includes a process by which
unsaved changes
andreset unsaved changes
functionality can be automatically built. This takes the form of auseReactEmbeddableUnsavedChanges
hook which:Comparator
for every piece of state defined in the factory's state type. Comparators are composed of:An
unsaved changes
subject is mandatory, but a subject initialized toundefined
can be supplied if this Embeddable does not have any state which could change over its lifetime.Other changes
This PR also makes some changes to the
publishingSubject
framework and updates all usages.T | undefined
only when either the subject itself, or the subject's contents could be undefined.Hello world code example
How to test
This PR creates an example React Embeddable in
examples/embeddable_examples/public/react_embeddables/eui_markdown_react_embeddable.tsx
. The best way to test is to use this React Embeddable, ensuring that all functionality works properly,Unsaved changes
,Cloning
,Copy to Dashboard
, setting titles & description etc.To get access to this React Embeddable you'll need to
yarn start --run-examples
. Additionally, there is no registered way to add one to a Dashboard yet. You can import a Dashboard which has one in place to start: