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

Allow constructors to return new objects. #95

Open
cscott opened this issue Feb 14, 2014 · 7 comments
Open

Allow constructors to return new objects. #95

cscott opened this issue Feb 14, 2014 · 7 comments

Comments

@cscott
Copy link

cscott commented Feb 14, 2014

The check in step 8 of CreatePromiseCapabilityRecord should be rewritten to:

\8. If Type(constructorResult) is Object then set promiseCapability.[[Promise]] to constructorResult.

This allows certain subclasses to be more easily written; see https://mail.mozilla.org/pipermail/es-discuss/2014-February/036293.html

@cscott
Copy link
Author

cscott commented Feb 14, 2014

This also allows Promise.all and Promise.race to be used with subclasses which want to have a unique constructor for each instance. The workaround from the above example (overriding Promise.resolve) doesn't work for these two methods.

cscott added a commit to cscott/prfun that referenced this issue Feb 14, 2014
The existing definition for Promise.all() doesn't permit this, really; we
need to reimplement it using _cast.  Also, we need to get rid of step 8
in CreatePromiseCapabilityRecord in order to use Promise.all/Promise.race.
(See domenic/promises-unwrapping#95).
Although we end up needing to reimplement all/race anyway, so perhaps
that's moot.
@domenic
Copy link
Owner

domenic commented Feb 14, 2014

This is bizarre. You should not have to create an entirely new class each instance. Something is wrong here, either in the spec or in your idea of the the implementation. "Defeating the optimization" sounds bad. I will be sure to look into this further.

In general constructors should never return anything; there is no point in making spec changes to accomodate such abberant subclasses. But it should of course be possible to build subclasses without returning things from subclass constructors.

@domenic
Copy link
Owner

domenic commented Feb 14, 2014

This was the monad-promise I was thinking. No weird return-from-constructors: https://gist.github.com/domenic/263d922475a7d7fa9414

@cscott
Copy link
Author

cscott commented Feb 14, 2014

I spent a good amount of time playing around with this tonight. You example is slightly broken; I've commented on the gist. The implementation at https://github.com/cscott/prfun/tree/monad-wip-1b actually works (a test suite and everything) but I end up having to reimplement Promise.all and Promise.race, which is too bad.

To restate: the desired characteristic is that the type signature of resolve is x -> Promise(x) (even if x is already a Promise), and that of then (or chain if you like) should be (handwaving) Promise(x) -> x, again even if x is a Promise. That is, resolve always adds one layer of wrapping and chain/then removes it. (The test cases for the implementation above specify exactly how much wrapping/unwrapping is done for a variety of different Promise combinator methods.) If you don't play games with unique constructors then step 2 of http://people.mozilla.org/~jorendorff/es6-draft.html#sec-promise.cast kills you, since it prevents Promise.resolve from always wrapping. I was trying not to have to override Promise.resolve (aka, "the method formerly known as Promise.cast") by ensuring that every constructor was unique, but you're right that it's easier just to bite the bullet and override Promise.resolve.

But even though I don't actually need it for Monadic promises, I still think returning a value from the constructor would be a useful and harmless ability to preserve.

@domenic
Copy link
Owner

domenic commented Feb 14, 2014

Well, Promise.resolve is a promise-level API (it lives in the then world); it is not a monad-level API. If you wanted to add flatMap, you would not use resolve as your monadic wrapper; you would use something like Promise.unit = x => Promise.resolve({ value: x }).

And yes, Promise.all and Promise.race operate on the promise level, not the monad level.

@cscott
Copy link
Author

cscott commented Feb 14, 2014

The odd thing is the the problem with Promise.all and Promise.race is that they do not unwrap enough. You can use resolve as your monadic wrapper with no problems. But all/race do exactly one resolve on each element and one then -- so the result is that the array elements aren't unwrapped at all. You can't really remove the resolve, since you need to cast to Promise. But if you add an extra promise = promise.then(v => v) to the spec then all/race work just fine. (In fact, see https://github.com/cscott/prfun/blob/monad-wip-1b/lib/index.js#L13-L15 .) And it's not hard to come up with reasonable monadic behavior for all the common combinators; the monad-wip-1b branch of prfun has all the details.

But in the end, I don't think the benefit of monadic use is worth the cost of forcing all the 'normal' users of all/race to suffer an extra then per element (and similar extra costs for other combinators you might write). So, whatevs.

This bug is actually about letting Promise constructors return objects, which I think is worth allowing even though it turns out that it's not key to a monadic Promise implementation.

cscott added a commit to cscott/prfun that referenced this issue Feb 14, 2014
The existing definition for `Promise.all`/`Promise.race` doesn't resolve
the array elements enough (we ought to `cast` instead of `resolve`), so
we need to reimplement them.

Haven't finished auditing the code and writing test cases to prove that
we unwrap exactly the right number of times in each method.

See domenic/promises-unwrapping#95
@allenwb
Copy link
Collaborator

allenwb commented Feb 14, 2014

Independent of whether or not it is a good idea to create a new class per instance as a work around for Promise.all/race behavior, the appropriate way to do it that doesn't run into step 8 iisues is:

It seems to me that in a complete ES6 implementation, the appropriate way to define this is using @@create:

class MonadicPromise extends Promise {
   static [Symbol.create] {
         return Object.setProtoypeOf(super(), (class extends this{}).prototype;
        // a new MonadicPromise subclass is needed for each instance,
        // in order to thwart the optimization in Promise.resolve
        // (formerly Promise.cast)
      }
      constructor(exec) {
          super((f,r) => exec( v => f([v]), r));
        }
    },
    then(f, r) {
        return super.then(v => f(v[0]), r);
    },
    // XXX Note that I wouldn't have to override this if it weren't for the
    // check in step 8 of CreatePromiseCapabilityRecord.
    resolve(v) {
        return new MonadicPromise(function(r) { r(v); });
    }
};

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

No branches or pull requests

3 participants