Skip to content
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

Defer factory should return ObservableInput, not just Subscribable | PromiseLike #2586

Closed
felixfbecker opened this issue May 2, 2017 · 10 comments

Comments

@felixfbecker
Copy link
Contributor

felixfbecker commented May 2, 2017

Observable.defer() takes a factory that takes Subscribable | PromiseLike. That is very annoying when the value is available synchronously and often results in a lot of boiler plate when usingconcat(). Simplified example:

const contents = new Map<string, string>();

getFilesInWorkspace()
  .mergeMap(uri =>
    // ensureContent emits no items, just completes once content is ensured
    ensureContent(uri)
      // After that, we can get the content synchronously
      // Need Observable.of() here because just returning an array is not allowed
      .concat(Observable.defer(() => Observable.of(contents.get(uri))))
  )
  .mergeMap(content => ...)
@kwonoj
Copy link
Member

kwonoj commented May 2, 2017

what's type of contents[uri]? is it Array<object> or Array<Observable<object>>? given snippet above does not provide information to actually reproduce issues.

@felixfbecker
Copy link
Contributor Author

Updated example.

@kwonoj
Copy link
Member

kwonoj commented May 2, 2017

Ok, so now I'm more confused. By subject, I guess Observable.defer(() => Observable.of(contents.get(uri))) is part you'd like to discuss, with // Need Observable.of() here because just returning an array is not allowed - is that correct?

but,

  1. comment says defer factory function returns array but code returns factory function returns string
  2. either case it doesn't work anyway cause defer expects function to return Observable object, so wrapping it via Observable.of is expected.

Would you clarify what's exact issue you're experiencing?

@felixfbecker
Copy link
Contributor Author

I would like defer() to accept ObservableInput, just like mergeMap, catch etc. do:

.concat(Observable.defer(() => [contents.get(uri)]))

@kwonoj
Copy link
Member

kwonoj commented May 2, 2017

OK, I see now.

I personally don't agree with those as it makes defer have polymorphic behavior by allowing ArrayLike<T> other than Observable itself. As you've already said those can be achievable trivially via current operator / static creation method set which doesn't require huge code boilerplate or technical difficulties to do so, I do not see huge value to it.

I guess I was confused by your subject stating Defer factory should return... makes me think some of incompatible type signatures are there around operators, but this seems more about feature suggestion. I'll update label and /cc @benlesh for second opinion.

@felixfbecker
Copy link
Contributor Author

But why does it accept Subscribable and PromiseLike then? It is already polymorphic, what is the benefit of not allowing ArrayLike? The concat+defer pattern is needed a lot when you want to have a promise.then-like chain of Observables where an Observable doesn't emit any items but only has a completion event.

@kwonoj
Copy link
Member

kwonoj commented May 2, 2017

Subscribable and PromiseLike is Observable-like, both can be subscribe into which isn't polymorphic in terms of subscribable behavior. All of operator which accepts observale can accept either subscribable or promiselike directly.

ArrayLike is not direct subscribable one need additional execution branching in code, reason I state it as make operator more polymorphic.

@felixfbecker
Copy link
Contributor Author

one need additional execution branching in code

Isn't the the extra branching required for ArrayLike the exact same extra branching required for PromiseLike? ArrayLike needs to check for typeof obj.length === 'number', PromiseLike needs to check for the presence of typeof obj.then === 'function'

@felixfbecker
Copy link
Contributor Author

Fixed in #2586

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants