Confusing error message when stream is expected

[haknick]

Summary

Wherever a stream is expected, if a Promise is passed (by mistake) instead, the message returned is very hard to understand.

Expected result

TypeError: Wrong type passed to 'most.chain' {insert func used}. Expected Stream, passed in Promise.

Actual Result

(node:41876) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): TypeError: Cannot read property 'run' of undefined (node:41876) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.

Versions

• most.js: 1.5.1 (all versions apply right now)

Code to reproduce

const most = require('most');

const shouldBeStream = foo => { return most.fromPromise(Promise.resolve(foo + 'bar')); };

most.of('foo') .chain(shouldBeStream) .observe(console.log);

[haknick]

This has already bit me twice. First time I blamed myself, the second I realized that in the midst of many code pieces this can happen more times than you think

[Frikki]

IMO, introducing runtime checks and custom error messages is expensive. Already, most has TypeScript types defined, which statically check for such errors. In the future, Flow types will also be available. If you have such concerns about correct types, I suggest you use TS if you can.

[Frikki]

Duplicate of #480

[TrySound]

These runtime checks can be placed only for development versions (like react, redux and others have) via process.env.NODE_ENV

[briancavalier]

I'm not sure how I feel about this. I understand the error coming from the VM is cryptic.

It's quite unhelpful that Node's unhandledRejection machinery appears not to be printing stack traces. There's nothing we could do about that. Even if we threw a descriptive error message, you'd never see the stack trace, due to node not printing it.

The only actual guarantee is a type system. If we check (x != null && typeof x.run === 'function'), there's no guarantee it's a Most.js Stream. Even instanceof isn't really a guarantee. The TS defs will prevent this code from ever running, and soon we'll have Flow defs (thanks to @TrySound) as well.

I'm open to finding a simple way to help as long as we don't have to pollute the code or take on much additional maintenance (e.g. in a build step). On the other hand, I'm not sure how much help we can realistically provide if Node is hiding the stack trace.

[TrySound]

We can try to introduce something like calls chain like promises do. We can use displayName property of functions. This will let at least to identify place by shape.

just.chain.map.switchLatest.tap.drain

[briancavalier]

I'm not in favor of adding that level of extra machinery. Also, it'd have to be done in a way that works across 3rd party libraries (which may export pure functions), which I don't think is possible.

I'd rather we all ask Node to improve their unhandled rejection reporting to include a stack.

When you see an unhandled rejection, you do have options for getting a stack trace. You can add a process unhandledRejection event handler, or you can .catch the promise returned by observe/drain/reduce. In either case, you'll be able to log the error stack trace, which should lead you reasonably close to the source of the problem.

[TrySound]

Wow, I tried to reproduce and seems like stack trace was improved in chrome. There's now a section in stack trace Promise resolve (async) which at least shows where observe was called. This can make debugging a bit simpler. However yes, we should force to use ts or flow to save performance.

[TrySound]

We can make a version with runtime checks for those who don't use type systems. It will also make stack trace more transparent, because checks throw errors inside constructors instead of delegated run methods.

[Frikki]

@TrySound

We can make a version with runtime checks for those who don't use type systems.

That’s extra maintenance.

[haknick]

For what it's worth, I completely agree with the reasoning above and in fact I wouldn't want to trade speed for clearer messages tbh.

This may be possibly fine in my case once Flow types are added and I might be able to use comment types.

I can close this issue if there's agreement to do so.

[davidchase]

I wonder if there is something we can do for those who dont not use TS/Flow... maybe in the form of docs or tips how to debug via the devtools?

[axefrog]

Just as an extra data point, React provides runtime checks in the dev-only build. I do the same in my own code in some places. I use tiny-preprocessor to strip out the dev-only checks for production. Disclaimer: not a request or an opinion about what Most.js should do in this regard.