Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

Promise(Like)-compatible RSVP type definitions. #42

Merged
merged 45 commits into from
Sep 17, 2017

Conversation

chriskrycho
Copy link
Member

@chriskrycho chriskrycho commented Sep 14, 2017

Reference

Exports

Derived from the list of all exports:

  • asap
  • cast – deprecated alias of resolve()
  • Promise
    • constructor
    • .all()
    • .catch()
    • .finally()
    • .race()
    • .reject()
    • .cast() – deprecated alias of resolve()
    • .resolve()
    • .then()
  • EventTarget
  • all
  • allSettled
  • race
  • hash
  • hashSettled
  • rethrow
  • defer
  • denodeify
  • configure
  • on
  • off
  • resolve
  • reject
  • map
  • async
  • filter

The RSVP namespace itself should simply be the default export, reexporting those items as an object.

@chriskrycho
Copy link
Member Author

chriskrycho commented Sep 14, 2017

Note: until we also rewrite the Ember type defs – again – to take only one type parameter for RSVP Promises, this will fail everything. UGH. I've taken a stab at that in f6e1c3e but it's possible I've missed something.

@chriskrycho
Copy link
Member Author

I fixed the remaining type defs issue mentioned above with e0243b8; at this point it should just be a matter of doing the (annoying, but mundane) work of adding the remaining definitions with tests. That's mostly just a lot of boilerplate to get it right—I noticed some quirks with the smart but ultimately not-sufficiently-polymorphic approach @theroncross and I took with e.g. RSVP.hash last time.

@@ -118,7 +118,29 @@ function testHash() {
}

function testHashSettled() {
// TODO: add test
function isFulfilled<T>(state: RSVP.PromiseState<T>): state is RSVP.Resolved<T> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is nice; I'm going to steal it and use it over in the allSettled in short order. I thought of doing something like this but got sidetracked looking at on and off and forgot when I got back to it.

@chriskrycho
Copy link
Member Author

I'm pretty sure denodeify is going to kill me. I'm seriously thinking of doing what Bluebird does and not even trying to do the variants which handle turning extra Node arguments into either hashes or extra items in a tuple.

@dwickern
Copy link
Collaborator

Let's start with some tests

@chriskrycho
Copy link
Member Author

chriskrycho commented Sep 17, 2017 via email

@chriskrycho
Copy link
Member Author

At this point, I'm pretty okay with where things stand. Unfortunately, I can't see a good way to accomplish two things with denodeify and this:

  • correctly requiring (not merely allowing) the this context to be bound if you do RSVP.denodeify(foo.bar) – the foo context gets lost.
  • adding additional arguments to the Node functions being denodeified, i.e. everything before the callback. Bluebird gets around this by supporting more of those but only one callback value. We could go either way, I think; but RSVP seems in its options biased somewhat toward supporting more callback values (since it has not one but two different ways to handle having more than one of those).

I think I'm good with merging this, doing a little local testing, and then upstreaming it with the relevant qualifications and caveats to RSVP proper.

@dwickern
Copy link
Collaborator

I was able to get this context working with a pair overloads like these (one for void, another for some generic this)

function denodeify<T1, T2, A, K1 extends string, K2 extends string>(
    nodeFunc: (this: void, arg1: A, callback: (err: any, data1: T1, data2: T2) => void) => void,
    options: [K1, K2]
): ( arg1: A) => RSVP.Promise<{ [K in K1]: T1 } & { [K in K2]: T2 }>;

function denodeify<This, T1, T2, A, K1 extends string, K2 extends string>(
    nodeFunc: (this: This, arg1: A, callback: (err: any, data1: T1, data2: T2) => void) => void,
    options: [K1, K2]
): (this: This, arg1: A) => RSVP.Promise<{ [K in K1]: T1 } & { [K in K2]: T2 }>;

@chriskrycho
Copy link
Member Author

Ah, awesome. I see what the problem was with mine; I was trying to do it with a generic This bound as void, and I can see how that wouldn't work. It wouldn't inherently know which overload to resolve.p since they're the same type arity.

@chriskrycho
Copy link
Member Author

chriskrycho commented Sep 17, 2017

Follow-up: if you can get a PR where it's working, I'll of course merge it, @dwickern, but my local experiments are not getting everything to play nicely across the board—possibly because of a combination of the type arity + declaration ordering issues TS has?

See #45.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants