feat(wire-service): rfc implementation #166
Conversation
- Add todo-api with getTodo (imperative) and wire adapter - Update single-wire demo to use new getTodo - Update build steps to work with typescript - Kill old code Outstanding: - analyze def wires to know which adapters depend on which vars - ctr the adapters, setup their wireUpdated callback
- More structure and types
| return { | ||
| updatedCallback: (newConfig) => { | ||
| config = newConfig; | ||
| subscription = getObservable(config).subscribe({ |
There was a problem hiding this comment.
Any time this code gets a new subscription, it should call subscription.unsubscribe() if subscription is defined and non-null. Wire adapter implementors may mess that up when dealing with raw Observables, so it's important to highlight that.
| buildContext | ||
| } from './wiring'; | ||
| export interface WiredValue { | ||
| data?: any; |
There was a problem hiding this comment.
can we be more specific here?
| removeEventListener(type: string, listener: WireEventTargetListener): void; | ||
| } | ||
|
|
||
| export type WireAdapterFactory = (eventTarget: WireEventTarget) => void; |
There was a problem hiding this comment.
We should probably rename this to WireAdapterCallback since it is not a factory anymore.
There was a problem hiding this comment.
it's not really a callback either, it's more like a setup... which factory kinda make sense, but i understand it's not returning a lifecycle hook object so it's not a factory.
| if (adapterFactory) { | ||
| const wireEventTarget = new WireEventTarget(cmp, def, context, wireDef, wireTarget); | ||
| adapterFactory({ | ||
| dispatchEvent: wireEventTarget.dispatchEvent.bind(wireEventTarget), |
There was a problem hiding this comment.
I'm not sure why is all these important. Eventually we can iron this out! not a big deal.
| * @param paramValues values for all wire adapter config params | ||
| */ | ||
| function invokeConfigListeners(configListenerMetadatas: Set<ConfigListenerMetadata>, paramValues: any) { | ||
| for (const metadata of configListenerMetadatas) { |
There was a problem hiding this comment.
avoid using iterators. let's use a regular for loop
|
|
||
| function updated(cmp: Element, prop: string, configContext: ConfigContext) { | ||
| if (!configContext.mutated) { | ||
| configContext.mutated = new Set<string>(); |
There was a problem hiding this comment.
do we really need a Set? can a regular array make the cut so we can just use for-loop on it?
There was a problem hiding this comment.
has to be a set to dedupe
| if (!configContext.mutated) { | ||
| configContext.mutated = new Set<string>(); | ||
| // collect all prop changes via a microtask | ||
| Promise.resolve().then(updatedFuture.bind(undefined, cmp, configContext)); |
There was a problem hiding this comment.
maybe an arrow here is better than an actual bind
Benchmark comparisonBase commit:
|
Benchmark comparisonBase commit:
|
Details
implements #148
Does this PR introduce a breaking change?