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 subclassing/extending Bluebird promises #1397

Closed
SystemDisc opened this Issue May 29, 2017 · 12 comments

Comments

Projects
None yet
6 participants
@SystemDisc
Copy link

SystemDisc commented May 29, 2017

Related to #325
Please consider allowing devs to subclass/extend Bluebird promises.

Current error:

TypeError: the promise constructor cannot be invoked directly

    See http://goo.gl/MqrFmX

Example use case (in TypeScript):

import * as Promise from 'bluebird';

export class PromiseWhile<T> extends Promise<T> {
	private valueWrap: () => T;
	private result: Promise<T>;

	constructor(callback: () => PromiseLike<T>, value: T);
	constructor(callback: () => PromiseLike<T>, value: () => T);
	constructor(private callback: () => PromiseLike<T>, value: any) {
		super((resolve, reject) => {
			setTimeout(() => {
				if (this.result) {
					resolve(this.result);
				}
				this.resolve = resolve;
				this.reject = reject;
			});
		});
		if (typeof value === 'function') {
			this.valueWrap = value;
		}
		else {
			this.valueWrap = () => value;
		}
		setTimeout(() => {
			this.runCallback();
		});
	}

	// in case resolve gets called before promise is set up
	private resolve(value?: T | PromiseLike<T>) {
		if (this.result === undefined) {
			this.result = Promise.resolve(value);
		}
	}

	// in case reject gets called before promise is set up
	private reject(value?: any) {
		if (this.result === undefined) {
			this.result = Promise.reject(value);
		}
	}

	private runCallback() {
		Promise.resolve(this.callback()).then((rVal) => {
			if (rVal == this.valueWrap()) {
				setTimeout(() => {
					this.runCallback();
				});
			}
			else {
				this.resolve(rVal);
			}
		}).catch((error) => {
			this.reject(error);
		});
	}
}

Example usage:

// doNextAsyncThing returns a Promise
// which resolves to true when there's more to do and false there isn't
// or rejects on error
new PromiseWhile(() => {
	return doNextAsyncThing();
}, true).then(() => {
	console.log('done doing async things');
}).catch((err) => {
	console.log('failed during async loop');
	console.trace(err);
});
@SystemDisc

This comment has been minimized.

Copy link

SystemDisc commented May 29, 2017

Also, the example code above works as expected if I remove || self.constructor !== Promise from

if (self == null || self.constructor !== Promise) {

@bergus

This comment has been minimized.

Copy link
Contributor

bergus commented Jul 2, 2017

This functionality sounds much like something that should be implemented as a function that returns an ordinary promise, not by subclassing Promise.

@SystemDisc

This comment has been minimized.

Copy link

SystemDisc commented Jul 3, 2017

I agree. It's probably a bad example. However, I think subclassing/extending bluebird promises should be allowed. We're all devs here...

@petkaantonov

This comment has been minimized.

Copy link
Owner

petkaantonov commented Oct 4, 2017

You should use scoped prototypes instead of subclassing to add functionality

@SystemDisc

This comment has been minimized.

Copy link

SystemDisc commented Oct 4, 2017

But why does your library explicitly block subclassing and extending? That seems really silly. Let the developers be in control. I don't see any valid reason to do this.

@arendjr

This comment has been minimized.

Copy link

arendjr commented Nov 22, 2017

I just ran into this issue trying to use child-process-promise in combination with Bluebird. Now I admit I do use global.Promise = require('bluebird') at the start of my program, which might be considered bad practice, but then again, Bluebird should be a drop-in replacement for ES6 Promises, whereas this issue causes it not to be. This basically prevents Bluebird to be used as a generic polyfill for ES6 Promises...

@SystemDisc

This comment has been minimized.

Copy link

SystemDisc commented Nov 29, 2017

This basically prevents Bluebird to be used as a generic polyfill for ES6 Promises.

Yes, it really does. It seems like some really arbitrary rule created and enforced by the bluebird developers. There's really no good reason for it. Let developers be developers.

@tyscorp

This comment has been minimized.

Copy link
Contributor

tyscorp commented Nov 29, 2017

Bluebird is over 3 years old at this point and was created in a different era of JavaScript. For whatever reason, @petkaantonov is either unable or unwilling to continue pushing this library forward. This is certainly acceptable and understandable; people's lives and priorities change and he holds no blame or responsibility in my opinion.

Bluebird was once extremely innovative and far ahead of its time, but I fear this is no longer the case. JavaScript engines and the JavaScript ecosystem have evolved. There are many issues with Bluebird's interoperability with modern build tools, practices and paradigms (webpack, ES6+, tree-shaking, etc.) that I don't see ever being fixed within Bluebird due to either ideological reasons ("You should use scoped prototypes instead of subclassing to add functionality") or just plain apathy.

Other than small bug fixes, I think it is safe at this point to consider Bluebird "finished".

@benjamingr

This comment has been minimized.

Copy link
Collaborator

benjamingr commented Nov 30, 2017

@tyscorp thanks for the comment.

bluebird is still faster than native promises, the v8 team uses it in benchmarks as promises are optimized - it's no secret that the eventual aim is to put ourselves out of business.

Both me and Petka are Node.js core and want to see native promises faster - there are discussions with engine developers (you can watch those at the Node and nodejs/promises repo) and things should get faster.

Bluebird is still incredibly stable and fast - it's de-facto the only choice for older browsers and older Node in my opinion. The utility methods are still very nice (native promises just got .finally).

Now that's out of the way, let's move to subclassing.


The subclassing issue: I believe there is absolutely no reason to subclass promises. This is not "ideological" as much as it is using promises for a wide variety of problems in a wide variety of settings.

I've discussed it with the people who specified the subclassing of promises in ECMAScript and they had the same idea. It sounded like a very interesting idea at first but I think everyone agrees that complicating the spec for it wasn't really worth it.

Subclassed promises are also significantly slower than native promises last time I checked in engines since they're so uncommon and there is little motivation to use them.

The main reason is that no matter what you do - an async function will still return the base class, coroutines will still return the base class and so will every language construct or API using promises.

Bluebird "won", promises have won, promises are more popular than callbacks and not in the fridge anymore. The future is async/await, async iteration and clean asynchronous flows.

I still use bluebird when I care about server performance, bluebird is still very compelling for other libraries to use - and Node.js apps. We've basically been adding or pitching bluebird features for native promises for the past few years and things that bluebird did like asynchronous stack traces, finally, unhandled rejection tracking and more have become standardized and adopted.

I'm not sure bluebird is 'finished', but I agree it's more in maintenance mode than in "adding features" mode.

ES6+ is still not optimized in some scenarios as well (although it's been getting much better since TF), tree shaking isn't needed since bluebird has its own partial build system which you can use instead and then require it with webpack. (If we're fair, tree-shaking in webpack doesn't work well yet anyway). One thing it's not is "apathy" :)

@SystemDisc

This comment has been minimized.

Copy link

SystemDisc commented Dec 1, 2017

This is literally the only line preventing extending Bluebird promises. If I remove this check, I'm able to extend and use Bluebird promises as expected.

if (self == null || self.constructor !== Promise) {

@benjamingr

This comment has been minimized.

Copy link
Collaborator

benjamingr commented Jan 26, 2018

So, JavaScript is looking into removing promise subclassing altogether.

@benjamingr

This comment has been minimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment