Conversation
|
Code LGTM, I am pulling and trying to integrate with #80832 |
lukeelmers
left a comment
There was a problem hiding this comment.
Did a full pass on the code, in general makes sense, I just had a few questions on some areas I wasn't sure about.
| export type PersistableStateDefinition<P extends SerializableState = SerializableState> = Partial< | ||
| PersistableState<P> | ||
| >; |
There was a problem hiding this comment.
Could you describe the changes here just a little bit more? I'm not 100% sure when you would want to use which type.
It looks like PersistableStateService and PersistableState are identical other than migrate vs migrations.
PersistableStateDefinition is now just a shortcut way of calling Partial<PersistableState>.
What would be the use case for when to use each? If all 3 are necessary maybe we can add some comments to clear this up.
There was a problem hiding this comment.
PersistableStateDefinition - is for definitions (embeddable factory, expression function etc), where things are mostly optional
PersistableState - actual items in the registry, where things are no longer optional but have defaults provided
PersistableStateService - service exposing a registry with persistable state items
yes, the main difference is that on definition thigns are optional, and the service exposes migrate function instead of migrations object
There was a problem hiding this comment.
Thanks for clarifying, makes sense.
My only minor nit then would be considering doing a shared interface for telemetry/extract/inject since they are the same, and then using the shared interface in PersistableState and PersistableStateService. But I don't feel too strongly on that; since they are already in the same file it should be pretty easy to remember to update it in both places if we ever need to
abb37cd to
c90abc2
Compare
| return { | ||
| state: updatedInput, | ||
| references: refs, | ||
| telemetry: getTelemetryFunction(commonContract), |
There was a problem hiding this comment.
Could we make this also available in setup phase because we will need this for server side SO migrations 🙏
lukeelmers
left a comment
There was a problem hiding this comment.
Code changes all LGTM! 🚀
|
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
* master: (127 commits) [ILM] Fix breadcrumbs (elastic#82594) [UX]Swap env filter with percentile (elastic#82246) Add platform's missing READMEs (elastic#82268) [Discover] Adding uiMetric to track Visualize link click (elastic#82344) [Search] Add used index pattern name to the search agg error field (elastic#82604) improve client-side SO client get pooling (elastic#82603) [Security Solution] Unskips Overview tests (elastic#82459) Embeddables/migrations (elastic#82296) [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663) Moving reinstall function outside of promise.all (elastic#82672) Load choropleth layer correctly (elastic#82628) Master backport elastic#81233 (elastic#82642) [Fleet] Allow snake cased Kibana assets (elastic#77515) Reduce saved objects authorization checks (elastic#82204) [data.search] Add request handler context and asScoped pattern (elastic#80775) [ML] Fixes formatting of fields in index data visualizer (elastic#82593) Usage collector readme (elastic#82548) [Lens] Visualization validation and better error messages (elastic#81439) [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490) chore(NA): install microdnf in UBI docker build only (elastic#82611) ...
Summary
adds migrate function to embeddable public api
each embeddable definition and embeddable enhancement can provide a list of migration functions per semver.
Checklist
Delete any items that are not applicable to this PR.
For maintainers