feat(combineLatest): support for observable dictionaries (#5022) #5363
Conversation
|
I updated the commit after an |
|
Anything missing or that I need to fix in this PR? |
|
@rraziel not sure if you saw the merge conflicts? |
|
Yes, they appeared few weeks after my PR, I was kind of hoping for someone to actually answer my questions before doing a rebase 😅 |
|
@joshribakoff alright, rebased and fixed the side-effect weirdness |
|
I'll resolve the conflicts when someone decides there is any point in having this PR, since there hasn't been any comment since the initial PR over 3 months ago. |
|
@benlesh @cartant @niklas-wortmann any thoughts on this? |
|
Rebased + api_guard files updated. |
Thanks. I'll review this later today. BTW, ignore the CircleCI failure. It's not effected by this PR. |
| }); | ||
|
|
||
| it('should work for the simple case', () => { | ||
| const res = combineLatest({ foo: of(1), bar: of('two'), baz: of(false) }); // $ExpectType Observable<{ foo: number; bar: string; baz: boolean; }> |
There was a problem hiding this comment.
I would prefer this test to use the helpers - a$, etc. - instead of of with literal values.
I can see that what's here is based on the forkJoin tests - which use of and literal values - but that's because those tests have not been refactored to use the helpers. Going forward, we should favour using the helpers.
There was a problem hiding this comment.
I see the helpers are - so far - only used in spec-dtslint/operators/*.spec.ts and not in spec/observables/*.spec.ts.
Can I just import those same helpers in spec/? (as they are in spec-dtslint/helpers)
There was a problem hiding this comment.
Nope. They're only for use - at least ATM - in the dtslint tests. They offer a somewhat more disciplined approach regarding the types - there are only a few different literal types, but we can add as many distinct 'helper' types as is necessary. With the tests in spec, we're less interested in the types and more interested in the notifications and their values and strings, etc. would be fine there.
|
Thank you, @rraziel! Very nice! |
Description:
Support for passing a dictionary of observables to
combineLatest, removing the need to pass an array then directly usingmapto turn it back into an object.I mostly based this on the similar work that was done on
forkJoin.Related issue (if exists):
#5022 and #5362