Create initial reference implementation #42
Conversation
|
Could even just convert it to JavaScript. I'm relatively indifferent. |
Finally getting to this! This is looking quite good, however I think I would prefer the reference implementation to be in pure JS if that's alright? |
Just a spike in TypeScript. Will add tests in a followup, add compilation to JavaScript, etc.
- moves license to LICENSE.txt - Updates reference impl to reflect proposed changes so far - Adds catch, finally, do, and switchMap
benlesh
force-pushed
the
reference-impl
branch
from
bc9d8ad to
f14f64b
Compare
|
@domfarolino should we merge this? is it useful? |
|
See #42 (comment). |
| eventType: K, | ||
| ): Observable<AbstractWorkerEventMap[K]>; | ||
| } | ||
| interface Animation { |
There was a problem hiding this comment.
Why are all of these being added?
There was a problem hiding this comment.
Honestly? Just to be nice. If anyone wants to use this with TypeScript, these types will help them. They don't need to be here.
There was a problem hiding this comment.
My two cents is we leave them. They can't do anything but help folks use this and try it out.
There was a problem hiding this comment.
Sorry I'm just not sure what they're actually doing. Can you help me understand what they add?
There was a problem hiding this comment.
They map the event name strings to the event subclasses. This way eventTarget.on('click') will typed as an Observable<MouseEvent> while eventTarget.on('keypress') will be typed as an Observable<KeyboardEvent>.
| }); | ||
| } | ||
| reduce(reducer, initialValue, options) { | ||
| return new Promise((resolve, reject) => { |
There was a problem hiding this comment.
Do we ever call reject here if signal gets aborted? I got here because I am spec'ing and implementing toArray(), and I was looking to find the semantics of what happens to the returned Promise when options.signal is aborted, but couldn't find it.
We certainly reject() in the error handling case specifically, but aborting options.signal shouldn't result in error actually being invoked (I think).
| eventType: K, | ||
| ): Observable<AbstractWorkerEventMap[K]>; | ||
| } | ||
| interface Animation { |
There was a problem hiding this comment.
Sorry I'm just not sure what they're actually doing. Can you help me understand what they add?
| } | ||
| } | ||
| if (errors) { | ||
| // TODO: We need to figure out how to report multiple errors. |
There was a problem hiding this comment.
A bug against the repo would be great to track this. I'm not sure I understand what this represents as-is, but some elaboration would help us solve it in the open I imagine. Alternatively, can't we just report each error individually? That's what the spec and impl does right now:
- https://wicg.github.io/observable/#ref-for-subscriber-teardown-callbacks%E2%91%A0
- https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/subscriber.cc;l=44-47;drc=e5f8ff312fe542f39c7d34b4440f18d1500b15f9
We should probably write a WPT for this case though.
| return this.#signal; | ||
| } | ||
| get isActive() { | ||
| return this.#active && !this.signal.aborted; |
There was a problem hiding this comment.
This doesn't seem to align to the current spec. AFAICT it's possible to see isActive === false while also this.signal.aborted === false.
| ...partialObserver, | ||
| }; | ||
| const subscriber = new Subscriber(options?.signal, observer); | ||
| this.start(subscriber); |
There was a problem hiding this comment.
Does this need to try/catch?
|
This PR really hasn't seen any activity in months, and with Chromium getting pretty close to shipping Observables, I feel like we should probably just close this PR out. But feel free to re-open if you have plans to get to it. |
Just a spike in TypeScript. Will add tests in a followup, add compilation to JavaScript, etc.