POC for persistable state service. #63085
Conversation
💚 Build SucceededTo update your PR or re-run it, just comment with: |
| id, | ||
| isDeprecated: !!isDeprecated, | ||
| extractReferences: extractReferences || identity, | ||
| injectReferences: injectReferences || noop, |
There was a problem hiding this comment.
all of them should have default implementation, that does nothing and all of them should not modify the object passed in (so injectReferences should also have identity rather that noop?)
| const definition = this.definitions.get(id); | ||
|
|
||
| if (!definition) { | ||
| throw new Error( |
There was a problem hiding this comment.
should we really throw here or just return undefined ?
| attributes: SavedObjectAttributes; | ||
| references: SavedObjectReference[]; | ||
| } | ||
| type ExtractReferences = (opts: SavedObjectWithReferences) => SavedObjectWithReferences; |
There was a problem hiding this comment.
should return saved object + reference array
There was a problem hiding this comment.
Why do we need attributes in addition to the reference array? For something like Expressions, that aren't backed by a saved object, what would attributes be?
| references: SavedObjectReference[]; | ||
| } | ||
| type ExtractReferences = (opts: SavedObjectWithReferences) => SavedObjectWithReferences; | ||
| type InjectReferences = (opts: SavedObjectWithReferences) => void; |
There was a problem hiding this comment.
should accept saved object + array of reference and return new object with injected references
| import { SavedObjectAttributes, SavedObjectReference } from 'src/core/public'; | ||
|
|
||
| export interface PersistableState< | ||
| S extends SavedObjectAttributes, |
There was a problem hiding this comment.
maybe this should not be refering to savedObjectAttributes ? there might be no saved object asociated with that state, for example saved visualization contains filters (which belong to data plugin, but are not saved on their own)
There was a problem hiding this comment.
Yeah I agree this is misleading. I used SavedObjectAttributes as a convenience because the type essentially allows for any serializable json, but should probably re-write the correct interface here to avoid confusion
| MS extends SavedObjectAttributes | undefined = undefined | ||
| > { | ||
| state: S; | ||
| migratedId?: I; |
|
I woke up this morning thinking about a use case that might put a wrench in this whole "use the id in place of the version number" scheme. We say today that it's only possibly for the owner of a saved object to migrate that saved object, but we do have an out. It's possible for us to write code to migrate every saved object. It's what security is going to do to migrate ids. We don't have that possibility if we use ids as the version numbers. Due to how flexible EmbeddableInput is, we could encounter this more. For example:
|
|
Filed my thoughts more formally here: #63358 |
|
closing in favor of #63451 |
Prior art: #57496
Summary
Summarize your PR. If it involves visual changes include a screenshot or gif.
Checklist
Delete any items that are not applicable to this PR.
For maintainers