fix(errors): Better error message when you return non-observable things,

[jayphelps]

item$.mergeMap(() => {})
// You provided 'undefined' where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.

item$.mergeMap(() => ({}))
// You provided an invalid object where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.

One thing I thought about doing is having operators (who are the ones who call subscribeToResult) pass in the operator name, so that the error message could also say which operator it thinks it occured in. But, what about operator aliases? i.e. mergeMap/flatMap, etc. And also, this could potentially be more confusing if it's a custom operator that calls other operators inside of it--but perhaps that's rare enough in the wild to discount it.

So for example: You provided 'undefined' to 'mergeMap|flatMap' where a stream was expected. You can provide an Observable, Promise, Array, or Iterable.

Cc/ @kentcdodds

[jayphelps]

If we can be sure that subscribeToResult is only ever used when you return something, I can use that word instead of "provided". It seems Observable.from does not use it

---

Edit: subscribeToResult is used by MergeAllSubscriber, which is used by Observable.merge(...streams), so using the term "return" could be confusing, instead of "provided" (or similar). Open to suggestions, if anyone has them.

[kentcdodds]

@jayphelps, you're one of my favorites <3

[jayphelps]

@kentcdodds hahaha well, I'm sure the others were gonna jump on this too. I literally already had a branch to change this that's been sitting for several months and your issue reminded me to finish.

This really is an annoyance I've had to deal with helping others on gitter, etc.

[kentcdodds]

Yeah, one thing that really helped me with beginner questions on angular-formly was validating every API with api-check. Would not recommend using this package anymore, but the concept of validating APIs for development (I was inspired by React) is awesome. I imagine TypeScript/Flow could really help with this :)

[jayphelps]

@kentcdodds Yeah, TS shouldn't let you return non-observable things. We could certainly consider adding more robust error checking wrapped in React-style process.env.node_env checks. Would you volunteer to create the ticket to track and (just maybe) give us some pointers?

[kentcdodds]

Can definitely create a ticket!

[coveralls]

Coverage increased (+0.001%) to 97.687% when pulling 307b888 on jayphelps:error into f51b8f9 on ReactiveX:master.

[robwormald]

We could certainly consider adding more robust error checking wrapped in React-style process.env.node_env checks

Could we avoid introducing any code like this that's going to require a specific build process? We have a ton of consumers of Rx not using Webpack, so I'd like to avoid penalizing them.

[kentcdodds]

Yes, that would be best. Optimally we should probably do what React's done by having the minified version exclude this stuff. So people not using a build system should use the minified version, and people using a build system can remove that themselves as part of the build. I wonder if there's a better way...

If I recall correctly, @jmdobry had a lib-name.debug.js file for his js-data library which you were supposed to use in development which had helpful things like this and then in production you'd use (and minify yourself) lib-name.js. Seems legit to me.

[kentcdodds]

All that said, one of the drawbacks of doing an "opt-in" system for this kind of thing is the people who generally need this kind of help wont know to opt into the help and you lose one of the main benefits of doing something like this in the first place. I think that making an opt-out version which removes this stuff is better.

[kentcdodds]

In any case, this is something that'll take a bit of effort, but it's well worth it IMO :) </done>

[jayphelps]

@robwormald @kentcdodds agreed. We can continue that discussion in #2153 💃

[benlesh]

It might be nitpicking, but one could return an Rx.Observable or a "lowercase-o observable" which is any object that implements Symbol.observable. How do we differentiate those two things there? Maybe Observable, Promise, Array, Iterable or object implementing Symbol.observable?

[jayphelps]

Yeah, I just punted on Symbol.observable because it felt like one of those "if you know it exists, you know it's supported" things. But I'm cool with adding of an object implementing Symbol.observable

[benlesh]

Also, can we just use a single template string \like this`` here instead of concatenating two strings together? I feel like the newline in the message is fine.

[jayphelps]

if you use template strings, the indention will matter and be wack. I would need to do this:

    const msg = `You provided ${value} where a stream was expected.
You can provide an Observable, Promise, Array, or Iterable.`;
// Notice how this can't be indented, otherwise the indention will also transfer through
// e.g. this:
    const msg = `You provided ${value} where a stream was expected.
      You can provide an Observable, Promise, Array, or Iterable.`;
// would output:
You provided null where a stream was expected.
      You can provide an Observable, Promise, Array, or Iterable.

If you're OK with either of these (or something else), I can change it. lmk. I may have to add an eslint ignore line, dunno.

[kentcdodds]

Please no. Please, leave it like it is :)

[benlesh]

I know it can't be indented. It just seems inefficient to allocate two strings, then concatenate them into another string. I'm okay with breaking the line length linting rule here too.

[benlesh]

then again, check the TypeScript compiled output, maybe if accounts for this and fixes it automagically.

[benlesh]

[benlesh]

LGTM

[lock]

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.