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

Provide means to extend Promise #325

Closed
rektide opened this issue Sep 24, 2014 · 30 comments
Closed

Provide means to extend Promise #325

rektide opened this issue Sep 24, 2014 · 30 comments

Comments

@rektide
Copy link

rektide commented Sep 24, 2014

Bluebird should be useable by people who wish to extend promises with other enhancements and things they want to try. At present this is made difficult because the Promise constructor throws an error if the constructor is not of type Promise.

I'd prefer to see a means to sidestep this constraint entirely, as it's a betrayal of the nature of prototypal inheritance. If someone wants to mixin all of the Promise methods and call it's constructor, JavaScript is a language designed to permit and allow that.

In a slightly more strict case, perhaps a this instanceof Promise call might suffice. I see this is way more strict than strictly necessary, but it's a starting place and a compromise which would allow Bluebird to be at least somewhat useful for people trying to experiment with promises.

The line in question which prevents Bluebird from being extended by anyone else:

if (this.constructor !== Promise) {

@benjamingr
Copy link
Collaborator

Have you seen the "for library authors"section?

On Sep 24, 2014, at 7:06 AM, rektide notifications@github.com wrote:

Bluebird should be useable by people who wish to extend promises with other enhancements and things they want to try. At present this is made difficult because the Promise constructor throws an error if the constructor is not of type Promise.

I'd prefer to see a means to sidestep this constraint entirely, as it's a betrayal of the nature of prototypal inheritance. If someone wants to mixin all of the Promise methods and call it's constructor, JavaScript is a language designed to permit and allow that.

In a slightly more strict case, perhaps a this instanceof Promise call might suffice. I see this is way more strict than strictly necessary, but it's a starting place and a compromise which would allow Bluebird to be at least somewhat useful for people trying to experiment with promises.


Reply to this email directly or view it on GitHub.

@rektide
Copy link
Author

rektide commented Sep 24, 2014

I'm following the advice in "for library authors," although I had missed it in my previous read throughs.

I've copy/pasted the constructor code into my own codebase to avoid the impossible to use constructor situation and everything works great, it's just really painful and I can see master and whatever the recent version I have via npm have already diverged, and it's sure to cause problems down the road.

As for quick ways to help other Promise library authors, I suggest perhaps make an _initialize method that the Promise constructor calls? Anyone else have ideas for what might be a good way forward to allow Promise to keep it's external interface the same while still allowing others to build off Promise?

@benjamingr
Copy link
Collaborator

What exactly are you building off Promise?

On Sep 24, 2014, at 5:41 PM, rektide notifications@github.com wrote:

I'm following the advice in "for library authors," although I had missed it in my previous read throughs.

I've copy/pasted the constructor code into my own codebase to avoid the impossible to use constructor situation and everything works great, it's just really painful and I can see master and whatever the recent version I have via npm have already diverged, and it's sure to cause problems down the road.

As for quick ways to help other Promise library authors, I suggest perhaps make an _initialize method that the Promise constructor calls? Anyone else have ideas for what might be a good way forward to allow Promise to keep it's external interface the same while still allowing others to build off Promise?


Reply to this email directly or view it on GitHub.

@stefanpenner
Copy link
Contributor

I know @petkaantonov has told me "he will not implement" but extending and subclassing are part of the promise spec, hopefully he re-considers.

@benjamingr
Copy link
Collaborator

@stefanpenner I'd love to hear some cool use cases for subclassing promises that aren't covered by what Bluebird currently does.

@rektide
Copy link
Author

rektide commented Sep 25, 2014

I really loath the "tell me your usecase before I am willing to consider a capability" party line. There's a billion ways you're going to be able to poke holes at the particular choices I made here (I made lazy choices about implementation (I should at least attempt to extend Defer, not Promise), and I do want to improve it, but I don't want to let you off the hook from facing what I feel is absolutely a core issue by providing a myriad of dodges to what is absolutely a core dilemna), but I made a lazy promise implementation.

Extending promises has got to be done. (Otherwise we're back in the old world of "native objects" that cannot be extended, and those were dark fucking times ya'll.) This check that I've highlighted is Anti-Javascript in nature, and there's no reason library authors should face such a brutal block.

http://github.com/rektide/laissez-bird

@benjamingr
Copy link
Collaborator

I'm not sure why the backlash. You're asking for an additional feature, where I'm trying to understand what's the problem with doing:

var Promise = require("bluebird/js/main/promise")();
Promise.prototype.someNewMethod = function(){ // add method to promise

};
Promise.someNewStaticMethod = function(){ // add new static method

};
return Promise;

If you want to swap out the constructor, or any particular method you're welcome to do that too through composition.

I'm not attacking you, I'm not suggesting 'poking holes' at what you're doing or making fun of you and I'm not sure why you'd even think that to begin with. I'm interested in understanding your use case, as I'm sure is @petkaantonov when you ask for an additional feature from the library. I'm also not a huge fan of your negativity.

About your implementation - it overrides internal methods like ._then that can change or disappear entirely at any moment from Bluebird, as is true for any other library whose internal undocumented parts you choose to override. If you want to override then I suggest you do that.

I'll let @petkaantonov take it from here.

@spion
Copy link
Collaborator

spion commented Sep 25, 2014

Hmm, I replaced the check with this instanceof Promise and all the tests pass. Have you tried forking bluebird, changing that line, then testing if your lazy promises library works? If it does, I think @petkaantonov will accept a PR for this... I tried looking at the git blame log and I can't really find a reason for the constructor check.

In any case, bluebird has some pretty scary internals (for performance reasons) and therefore its not the friendliest library to do inheritance experiments with. Add to that lazy promises having very different semantics... personally I'd avoid inheritance entirely in this case.

@stefanpenner
Copy link
Contributor

@spion I think it may be more complex then that.

Stuff like: https://github.com/petkaantonov/bluebird/blob/master/src/promise.js#L338 would also need an indirect reference to the subclasses constructor.

Something like var Constructor = this.constructor could be used

https://github.com/petkaantonov/bluebird/blob/master/src/promise.js#L266 would need to be a strict equality check

etc.

@spion
Copy link
Collaborator

spion commented Sep 25, 2014

Yep, it may be - thats why I suggested trying it out first and seeing where it goes :)

@benjamingr
Copy link
Collaborator

@stefanpenner that shouldn't be too hard to do. I think @petkaantonov doesn't really believe in extending through subclassing though.

@stefanpenner
Copy link
Contributor

@spion I don't think it would be very involved actually, and I'm pretty sure the perf difference would be non-existent.

@stefanpenner
Copy link
Contributor

@benjamingr yup he doesn't, and I know this will likely just bug him further (i have bugged him about this on several occasions)

Also your question before was a trap, so I didn't answer it :P

I'm not sure I can think of any scenarios where require('..')() doesn't also work.

But I believe the goal for promises to be sub-classable is to align them with language level subclassing semantics that hopefully become ubiquitous. Subclassing HTMLElement, Array, Promise etc.etc. via the same mechanism is appealing.

a future of (excuse my likely invalid es6/es7 syntax):

class SpionPromise extend Bluebid {
  then (a, b) {
    spionInstrumentation(a,b, "something");
    return super.then(a,b);
  }

  spionize () {
     // the next cool stream/promise experiment spion dreams up
  }
}

vs

var SpionPromise = require('..')();
SpionPromise.prototype._super$then = SpionPromise.prototype.then;
SpionPromise.then = function(f,r) {
  SpionInstrumentation(f, r);
  return this._super$then(f, r);
}

is appealing.

Especially when in theory existing natives and other libraries all extend the same way.

maybe @domenic, @dherman or @wycats can share some thoughts on the matter.

@stefanpenner
Copy link
Contributor

Subclassing does impose some runtime performance annoyances, but only when mixing different subclasses instances.

as "legally"

LabelledPromise.resolve(progressPromiseInstance) instanceof LaballedPromise

and

LabelledPromise.all([progressPromiseInstance]) instanceof LaballedPromise

and this translation best case will still come at a cost of at least an additional allocation and at worse a formal assimilation process of a foreign thenable.

But I believe those exact issues exist using bluebirds current require()() method. So this is likely a neutral point.

@benjamingr
Copy link
Collaborator

@stefanpenner Right, and when class gains even the slightest adoption in any JS community (node, browsers, whatever) - I think that @petkaantonov might need to reconsider.

Until then, no one is likely going to do class MyPromise extends Bluebird anyway, a TypeScript or CoffeeScript argument might be better for subclassable promises. Over here in JS land people are simply not using their objects this way yet. Not to mention that again, we already have a way to extend promises in Bluebird.

This is the first time I'm aware of user actually wants to subclass Bluebird, and aside from some snarky remarks like calling Bluebird "fascist miserable adamntitely awful software", he didn't really explain his rationale or provide a solid use case why what we have now doesn't work.

@benjamingr
Copy link
Collaborator

@stefanpenner

LabelledPromise.resolve(ProgressPromise) instanceof LaballedPromise

I'm not sure how native promises subclassing handles this, but this sounds like plain old thenable assimilation, taking an untrusted thenable and converting it to a regular promise. Bluebird actually has an optimization for that (here).

@wycats
Copy link

wycats commented Sep 25, 2014

@benjamingr For what it's worth, the ES6 class syntax is a relatively straight-forward syntax for an existing, and commonly used pattern:

class LabelledPromise extends Promise {

}

// means this and a bit more

var LabelledPromise = function() {
  Promise.apply(this, arguments)
}

LabelledPromise.protoype = Object.create(Promise.prototype);

That pattern is actually reasonably common, and wrapped up in a lot of libraries that implement class-like abstractions.

When the Promise spec says that it's subclassable, it's specifically talking about the latter pattern, which the class syntax is sugar for.

@benjamingr
Copy link
Collaborator

@wycats Thanks, I'm aware of that. I was specifically referring to @stefanpenner's comment about the syntax being superior and pointing out the cultural problem at the moment with the syntax.

Given that (as far as I know) browser promises are not yet subclassable and neither are plenty of other libraries, and given that this is the first user in a year that has asked for this feature. I don't really understand the reason to implement this. Bluebird also does provide a way to extend it.

@stefanpenner One example of a problem with subclassing Bluebird is that it would require each subclass to implement a version of the promise constructor that accepts the INTERNAL argument and provide a method for fast creation without allocating a closure for each promise created. I'm not sure how that could be specified well (requiring effectively two overloads of the constructor, and require Bluebird to expose the INTERNAL out (hah)). This would be very confusing IMO to people who'd expect to just subclass promises and frankly I think composing or decorating are much simpler.

If we don't ""force"" people to implement that we'd have to fall back to allocating a closure for every promise created through a subclass, and doing something like MyPromise.promisifyAll(module) would be much slower.

@stefanpenner
Copy link
Contributor

I'm not sure how native promises subclassing handles this, but this sounds like plain old thenable assimilation, taking an untrusted thenable and converting it to a regular promise. Bluebird actually has an optimization for that (here).

Ya but this flag is problematic when subclassing.

For example, if someone creates a lazy then implementation sub-class as the OP describes (or even my spionized promise for instrumentation) the expectation would be that given ParentClass.resolve(subclassInstance) ParentClass consume subclassInstance via the public interface of then not the private interface of _then. This unfortunately leads us back to the slow but thorough formal assimilation process.

In theory rather then using a hidden property, if then function equality was used to detect tampering of the public interface and we based the ideal code-path on that branch, we could avoid some of the issues in some scenarios. But not in either of the extension examples above.

Since the promise story is all about library inter op, it is truly unfortunate but the current public API for spec compliant promises is insufficient to implement performant subclass inter op.

@stefanpenner
Copy link
Contributor

@stefanpenner One example of a problem with subclassing Bluebird is that it would require each subclass to implement a version of the promise constructor that accepts the INTERNAL argument and provide a method for fast creation without allocating a closure for each promise created. I'm not sure how that could be specified well (requiring effectively two overloads of the constructor, and require Bluebird to expose the INTERNAL out (hah)). This would be very confusing IMO to people who'd expect to just subclass promises and frankly I think composing or decorating are much simpler.

this is exactly my point, composition breaks down here as well, unless you begin to utilize and override bluebird internals.

Just to be clear, I'm not saying bluebirds current implementation of extension is inherently better or worse then extend I am merely pointing out that subclassing/extension etc has a pretty narrow path of success without regressing on performance.

I hope a better solution is possible, or people may avoid subclassing/extension all together which would be sad.

@benjamingr
Copy link
Collaborator

In theory rather than using a hidden property, if then function equality was used to detect tampering of the public interface and we based the ideal code-path on that branch, we could avoid some of the issues in some scenarios. But not in either of the extension examples above.

That would not work between different versions of Bluebird though, the optimization was for people to be able to use and assimilate promises from libraries that use different versions of Bluebird themselves quickly.

this is exactly my point, composition breaks down here as well, unless you begin to utilize and override bluebird internals.

Decoration doesn't though. If you extend Bluebird the way Petka describes in the library author guide - you'll get fast thenable assimilation from other bluebirds, formal but slower thenable assimilation from other libraries, and fast promisification.

The only thing you sacrifice is the constructor, which you can easily swap out for something like a Promise.make(resolve) function you can use and/or expose in your library.

@stefanpenner
Copy link
Contributor

Decoration doesn't though. If you extend Bluebird the way Petka describes in the library author guide - you'll get fast thenable assimilation from other bluebirds, formal but slower thenable assimilation from other libraries, and fast promisification.

Yes this is the only solution, to avoid the public promise api is to submit to a side-channel protocol. Which is the point I'm trying to make, I am excited you acknowledge this.

The problem is, having to resort to a side-channel is truly unfortunate.

This makes me incredibly nervous about the potential success of native implementations in the future. Assuming they can (and they clearly can) achieve the same or great performance the the fastest libraries today. As soon as you want to extend, you will be penalized and will very likely prefer using a user land implementation.

@petkaantonov
Copy link
Owner

Proper subclass support as specced cannot be implemented efficiently, you need to use a lot of reflection and try catches everywhere as you try to see if someone did crazy things like overrode this.constructor with a getter that throws. On top of that actually using a subclass would also disable most optimizations.

You can extend promise as described in the for library authors section. However, how would the initialize method work? When a promise is created internally, a resolver function doesn't even exist.

@stefanpenner
Copy link
Contributor

Proper subclass support as specced cannot be implemented efficiently

I'm not sure it is even possible to implement (in user-land) as spec'd. Regardless of efficiency.

I am just emo that the spec doesn't enable efficient extending (composition or inheritance).

@benjamingr
Copy link
Collaborator

@stefanpenner raise an issue for ES7 on ESDiscuss to expose an interface for fast subclassing of Promise through a symbol.

Also - see domenic/promises-unwrapping#87 for more details.

@stefanpenner
Copy link
Contributor

Ya that likely is a better venue. Although I have made people aware of my concerns but it doesn't appear much interest exists.

@opensourcejunkie
Copy link

Hey, sorry to reopen a dead thread; I'm about to start working on a library that extends bluebird, and I want to ensure I'm doing it the best way. Per this discussion, and the "For library authors" section, it sounds like extending bluebird by decoration is still the best method. Does anyone know anything to the contrary?

Thanks :-)
~ OSJ

@nmccready
Copy link

It's been a long time since this thread started; but now since we're on 3.X.. where did this documentation of "library authors section" go to? Anyone have the url?

@nmccready
Copy link

nvm ugh #1040

@SystemDisc
Copy link

SystemDisc commented May 28, 2017

I'd really like subclassing to be allowed.

Edit: moved example code to new issues, since this issue is closed. See: #1397

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

No branches or pull requests

9 participants