promise take 1

[gustavnikolaj]

I picked up this project again, after I rediscovered the idea about duplex streams that you already had discussed ( #7 ). I started changing stuff on master, and now I ran into something while adding promise support, that I'd like your thoughts about.

Looking at the change to test/index.spec.js, the test is currently written with as a traditional mocha async test. That is because of the error I got from the type system when trying to use unexpected:

return expect(jshell('echo foobar'), 'to be fulfilled with', new Buffer('foobar\n'));

expected Duplex to be fulfilled with Buffer([0x66, 0x6F, 0x6F, 0x62, 0x61, 0x72, 0x0A])
  The assertion 'to be fulfilled with' is not defined for the type 'Stream',
  but it is defined for the type 'Promise'

I get why this is happening. I don't really return a Promise, just a thenable. But shouldn't that be enough to work with the Promise assertions in unexpected?

I cannot make the object that the jshell function returns both an instance of Duplex Stream and an instance of Promise. But just making it a thenable allows me to keep it nice and simple and there's not really any ambiguity about what the intention of the caller is. Or is there?

/ping @sunesimonsen @papandreou

[sunesimonsen]

@gustavnikolaj a thenable should be enough.

[gustavnikolaj]

Unless my memory of the Promise spec is flawed, my object should count as a thenable. And it seems that it is even enough to identify as a Promise in unexpected https://github.com/unexpectedjs/unexpected/blob/master/lib/types.js#L725

I guess the problem is that my object is identified as a Stream first, and that unexpected cannot handle something being more than one type of thing as the same time?

[papandreou]

I just ran into pretty much the same thing: https://github.com/unexpectedjs/unexpected-express/commit/2f1d57b5f1b027bc91305a60cfd8c8a75c3134f2

We only identify the most specific type of the subject, in this case Stream as unexpected-stream is installed after the core types. That doesn't take into account that the subject can also be identified as a Promise, which isn't configured as a base type for Stream (because it won't always be).

[papandreou]

It seems like expect should backtrack, try to identify the subject as one of the remaining types, then look for matching assertions again before giving up.

[gustavnikolaj]

Ah yes. It would not make sense to make it a base type...

So, I could fix this by creating another instance of unexpected without the streams plugin.

It might be enough of an edge case that it's not worth adding support for.

[sunesimonsen]

@papandreou that sounds dangerous

[sunesimonsen]

I don't have the time to look at this issue right now. But I'll look into the case later.

[gustavnikolaj]

I'm about to leave for a birthday-tour in the family, so don't sweat it. I'm even okay with you just saying that this is too obscure to solve - I don't mind having to create another instance of unexpected. I just wanted to touch base and make sure that I wasn't missing something :-)

[papandreou]

@sunesimonsen Might be dangerous for types that truly need to be "exclusive", but I can't think of any?

[sunesimonsen]

@papandreou I just think it would yield unpredictable behaviour.

[sunesimonsen]

This is strongly duck typing with precedence, I don't think that will ever work in a desirable way. I think you should create a type for the duplex stream that inherits from stream and just add the assertion to be fulfilled with for that type. You need to decide if this is a stream or a promise or both.

[papandreou]

There's definitely a use case here, I think. A lot of custom types are also streams or promises (in this case both), but they can't inherit from both, and Stream is not a core type. These mixins, traits, or whatever really seem like they should work as an supplement to the type hierarchy in terms of which assertions are applicable.

[sunesimonsen]

I understand use case and I think it is valid, but I just don't think it will align with our type system. It is multiple inheritance or duck typing and our type system is single inheritance and last definition wins. I think it would be quite weird if the hacked duplex object would be identified by assertions as a promise, but would have the stream type in other cases. I just think that would pave the road for a lot of wtfs.

I just tried it, as it is actually what the first implementation of typed assertions did. It only used identify of the types in the assertion. The experiment made 42 tests fail in weird ways: https://github.com/unexpectedjs/unexpected/tree/spike/v10-duck-typing

[sunesimonsen]

@gustavnikolaj to solve your concrete problem, try making two sibling describes one for streaming functionality and one for promise functionality. In the streaming describe you make a before function that installs the unexpected stream plugin. I know this is not optimal but is will keep you going.

[papandreou]

@sunesimonsen What I had in mind was more along these lines: https://github.com/unexpectedjs/unexpected/tree/feature/v10-duck-typing-with-less-impact

Basically this approach is the same as on feature/v10 except that it tries identifying the subject as the next type if no compatible assertion was found. So it only makes a difference in cases that would otherwise have resulted in the "No matching assertion" error.

It makes no Unexpected tests fail, and it makes Gustav's promise+stream test work with the unexpected-v10 branch of unexpected-stream linked in.

(You could of course argue that for the sake of consistency, the same approach should apply to the arguments, but one thing at a time :)

[sunesimonsen]

I'm not completely against making the assertions works as typed pattern matching. But There is one problem with your proposal. If an assertion is chosen that identified the duplex stream as a promise and that assertion would use expect.findTypeOf(subject) you'll get the stream type. It you expect to get the promise type and call a method on it, it would fail.

I fixed a problem on my spike so now I only have 3 failing tests, all of them is about the above problem. So if we chose to do this, we need to get rid of expect.findTypeOf or make the assertion types available as already proposed. My approach will be much faster than trying all the combinations of subject and argument types that can identify the values and should yield the same result. I tried to use it in combination with Gustav's branch and it fixes the problem his is experiencing.

What do you think @papandreou?

[papandreou]

@sunesimonsen That sounds good, especially if it is likely to perform better. Also, I assume that your proposal does solve the consistency problem with my proposal that I outlined in my last comment? That is, the below would also work despite unexpected-stream classifying the jshell instance as a stream:

var expect = require('unexpected').clone().use(require('unexpected-stream'));
expect.addAssertion('<any> to be the fulfillment value of <Promise>', function (expect, subject, value) {
    return expect(value, 'to be fulfilled with', subject);
});

it('should work the other way around', function () {
    return expect(new Buffer('foobar\n'), 'to be the fulfillment value of', jshell('echo foobar'));
});

[sunesimonsen]

Hmm my proposed solution wont work and I don't thinks yours will either. If you use the assertion subject type for getKeys for example, you will not get the inherited version, that is an error. I don't think this will work.

[sunesimonsen]

Hmm you solution takes care of this problem, let me think some more about it.

[sunesimonsen]

@papandreou I tried a few more things and I can't get my solution to work for all cases. I think it is a requirement that we try to find other type candidates for the arguments as well, otherwise it would be inconsistent. I find the solution you proposed it too slow to be a viable option as you would need to try all type combinations of the subject and arguments. Furthermore it would break backward compatibility of findTypeOf. So I would like to postpone the discussion of this feature to a later release and stick with what we have for v10. Are you okay with that?

[papandreou]

The fact that we're reworking the types for v10 makes it seem like a now-or-never thing, although on the other hand it's not really a regression. If you feel that the eventual solution, whatever it might be, won't cause us to backtrack on some of the v10 changes, I'd be ok with it.

We have some not-so-pure workarounds available, such as defining all the promise assertions for any. Seems like a step backwards, though.

[sunesimonsen]

I don't think this requires hacks from core, it is just a use case that is not supported, like it wouldn't be supported by any mainstream type system. What I'm afraid is that we will not be able to come up with a solution that is fast enough to be okay.

But I of cause agree that it is a desirable feature.

[papandreou]

like it wouldn't be supported by any mainstream type system

For promises and streams, the best analogy to other type systems would probably be interfaces, and Java, Go etc. permit that one class implements multiple ones. Maybe we need another type of type with different semantics?

[sunesimonsen]

I haven't completely given up on making my solution work, I have some new ideas. Go does not support inheritance, but what we are trying to achieve here matches pretty well with Go's static duck typing. With interfaces you still need to state that you implement them.

[sunesimonsen]

@papandreou I might have cracked it: https://github.com/unexpectedjs/unexpected/tree/spike/v10-duck-typing

[gustavnikolaj]

You guys really did a lot of work!

I can work around this, and I don't consider this a must-have feature, but it's definitely in the nice-to-have category. As a naive user of unexpected and JavaScript, I would just expect it to work.

But when that is said, the v10 branches of unexpected and unexpected-stream makes this work beautifully. :-)