-
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
Common Registry
interface
#39059
Common Registry
interface
#39059
Conversation
💚 Build Succeeded |
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.
Love this PR.
Where is freeze
going to be used?
Would be used like Tim suggested to freeze registries after setup life-cycle. |
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.
The PR looks great.
I'm a bit skeptical of having freeze here, as there's no immediate use.
But @streamich mentioned @timroes and @epixa want it for some planned uses.
So LGTM.
💚 Build Succeeded |
filterBy, | ||
set, | ||
clear, | ||
set$: (set$ as unknown) as Observable<T>, |
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.
What would this be used for?
Also if you define a type parameter for the new Subject<T>()
above, you should be able to use the .asObservable()
method instead of having to cast like this. That will also ensure that there is no access to the next
method outside this module.
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.
What would this be used for?
Most likely user would be actionsRegistry
in Embeddables where in Phase II users can add actions dynamically at runtime.
Also if you define a type parameter for the
new Subject<T>()
above, [..]
Yeah, I forgot <T>
.
That will also ensure that there is no access to the
next
method outside this module.
That's why I'm upcasting to Observable<T>
it "removes" .next()
method without creating a new observable.
import { Observable, Subject } from 'rxjs'; | ||
import { Registry, Predicate } from './types'; | ||
|
||
export const createRegistry = <T>(): Registry<T> => { |
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.
Overall, I primarily want to make sure we're not solving a more complicated problem than we need to. There are some choices that I'm a bit unclear about. What is the advantage of using generators instead of simply returning arrays? Do we benefit from providing an id when adding items? Why provide separate observables for set and clear?
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 guess we discussed registries at our meeting already. But for posterity:
What is the advantage of using generators instead of simply returning arrays?
This implementation handles all our registry use cases (uiRegistry
, kbn-interpreter
registries, and other ad-hoc registries).
Do we benefit from providing an id when adding items?
Because in all registries (I believe) records can be identified by ID, and in some uiRegistries
and all kbn-interpreter
registries records are also accessed by ID. So, registries are effectively key-value stores.
Why provide separate observables for set and clear?
🤷♀️ Just one way of doing it.
Closing this as we went ahead without a common registry implementation instead: #39168 |
Summary
Registry
interface.IndexPatternCreationConfigRegistry
uiRegistry
byRegistry
— @mattkimeRegistry
interface for all Embeddables registries — @stacey-gammonChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers
This was checked for breaking API changes and was labeled appropriately