-
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
[embeddable] remove embeddable factory methods from setup and start API #204797
[embeddable] remove embeddable factory methods from setup and start API #204797
Conversation
/ci |
/ci |
/ci |
/ci |
/ci |
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.
Changes LGTM! Left a few questions. Code only review. It's a good sign that we didn't have to remove or modify functional tests.
@@ -15,7 +15,7 @@ import { injectBaseEmbeddableInput } from './migrate_base_input'; | |||
export const getInjectFunction = (embeddables: CommonEmbeddableStartContract) => { | |||
return (state: EmbeddableStateWithType, references: SavedObjectReference[]) => { | |||
const enhancements = state.enhancements || {}; | |||
const factory = embeddables.getEmbeddableFactory(state.type); | |||
const factory = embeddables.getEmbeddableFactory?.(state.type); |
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.
Looks like this function was marked optional in CommonEmbeddableStartContract
, but that type isn't used in the client-side definition of EmbeddableStart
. I understand this function was removed from the clientside version, but is there a reason it needed to be marked optional in the serverside one?
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.
Both client and server use inject
so I typed getEmbeddableFactory
as optional since its not provided by the client.
private appList?: ReadonlyMap<string, PublicAppInfo>; | ||
private appListSubscription?: Subscription; | ||
private enhancementsRegistry = new EnhancementsRegistry(); |
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 to move the registry code into its own file rather than clogging up the plugin.
}; | ||
|
||
const getAllMigrationsFn = () => | ||
getAllMigrations( | ||
Array.from(this.embeddableFactories.values()), | ||
Array.from(this.enhancements.values()), | ||
[], |
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.
Nice temporary solution here
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
🤖 GitHub commentsExpand to view the GitHub comments
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 owners review)
@elasticmachine merge upstream |
⏳ Build in-progress
History
|
Starting backport for target branches: 8.x |
…PI (elastic#204797) Part of embeddable rebuild cleanup. PR removes legacy embeddable factory registration APIs. --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> (cherry picked from commit 3b61e7b)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…tart API (#204797) (#204993) # Backport This will backport the following commits from `main` to `8.x`: - [[embeddable] remove embeddable factory methods from setup and start API (#204797)](#204797) <!--- Backport version: 9.4.3 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Nathan Reese","email":"reese.nathan@elastic.co"},"sourceCommit":{"committedDate":"2024-12-19T18:19:48Z","message":"[embeddable] remove embeddable factory methods from setup and start API (#204797)\n\nPart of embeddable rebuild cleanup. PR removes legacy embeddable factory\r\nregistration APIs.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"3b61e7bea7408e586faa4be6464e963b50385896","branchLabelMapping":{"^v9.0.0$":"main","^v8.18.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Embedding","Team:Presentation","release_note:skip","v9.0.0","ci:project-deploy-observability","Team:obs-ux-infra_services","project:embeddableRebuild","backport:version","v8.18.0"],"title":"[embeddable] remove embeddable factory methods from setup and start API","number":204797,"url":"https://github.com/elastic/kibana/pull/204797","mergeCommit":{"message":"[embeddable] remove embeddable factory methods from setup and start API (#204797)\n\nPart of embeddable rebuild cleanup. PR removes legacy embeddable factory\r\nregistration APIs.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"3b61e7bea7408e586faa4be6464e963b50385896"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/204797","number":204797,"mergeCommit":{"message":"[embeddable] remove embeddable factory methods from setup and start API (#204797)\n\nPart of embeddable rebuild cleanup. PR removes legacy embeddable factory\r\nregistration APIs.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>","sha":"3b61e7bea7408e586faa4be6464e963b50385896"}},{"branch":"8.x","label":"v8.18.0","branchLabelMappingKey":"^v8.18.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT--> Co-authored-by: Nathan Reese <reese.nathan@elastic.co>
Part of embeddable rebuild cleanup. PR removes legacy embeddable factory registration APIs.