-
Notifications
You must be signed in to change notification settings - Fork 95
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
Make subclassing work automatically #31
Conversation
This allows subclasses to override `then` without having to override `catch`; it will instead just automatically do the right thing. Note that this is an observable change, as now overridden `then` methods will be called when you use a non-overridden `catch`.
What does "real constructor" mean? |
It would mean however the promise was created, i.e. if it was created via |
These are relatively easy to figure out. If they are being called on a constructor, e.g. `CancellablePromise.race(...)` or even `Promise.race.call(CancellablePromise, ...)`, then pass a resolver to the subclasss that extracts `resolve` and `reject` methods, and use those instead of using spec-internal mechanisms.
It first checks that `x.[[IsPromise]]`, which will be true for all `Promise`s and for all ES6 `Promise`-subclass instances. Then, it ensures that it only passes through promises for which `x.[[PromiseConstructor]] === this`, so that e.g. `Promise.cast` only passes through promises derived from `Promise[@@create]()`, whereas `CancellablePromise.cast` only passes through promises derived from `CancellablePromise[@@create]()`.
It now automatically returns an instance of the subclass. Effectively, it constructs it via `new this.constructor(() => {})` and then uses internal magic to resolve or reject the returned promise. This is unlike the static methods which save `resolve` and `reject` from the resolver to perform their work. But this seems OK, as `then` is so fundamental, that you should not be able to add new behavior to it by passing in modified `resolve`/`reject` arguments to the resolver in your subclass.
Updated to address feedback, including adding the new |
Could the requirement for checking if reading the property I have seen issues that have actually no, or marginal, effect on performance drive some decisions so I hope this feature which appears to actaully have no practical use case could be removed. |
@petkaantonov unfortunately I don't think it can be relaxed. We need to guarantee that |
Implementers in C++ should have access to some tricks, like optimistically assuming that |
I'm starting to look at what's involved in creating a CancellablePromise subclass which adds a new cancel method to the promise and follow the behavior we worked out at Promises/A+. One thing I wonder is if there needs to be sub-class affordances for the case where a sub-class is resolved with another promise that is not the same sub-class. For example:
The rough behavior I'm looking at for Cancel is... Cancel(p)
|
@skaegi sorry for the delay in replying, let's see here...
You should definitely still be able to cancel the promise;
Hmm, I think I see what you're trying to do here. In contrast to the proposal at promises-aplus/cancellation-spec#6, it would make the following work: const cp = new CancellablePromise(() => { }, () => console.log("cancelled cp"));
const cp2 = new CancellablePromise(resolve => resolve(cp));
cp2.cancel(); // causes "cancelled cp" to be logged. which seems like a good thing. Very nice. So... I guess, I'm not sure what affordance you would be looking for? Things seem to work pretty well as-is? |
@skaegi I just had a related thought, or perhaps it was the same thought you were having and I didn't quite understand. Namely... how can you do subclassing of the type you want, in pure JS? We want to make Can you think of a way to implement your cancel operation without using the internal properties? If not, that may mean that we need to expose them to subclasses somehow. |
Yes, probably easiest to talk in terms of code so let me show you my ES5 implementation (which is using your new "then" sub-classing technique which helps). It's based on sub-classing Deferred but I'll put up a repo when I get a moment to show an implementation based on Promise.
This implementation passes the tests I originally wrote at Promises/A+ (https://gist.github.com/skaegi/4736504) as well as the Orion Deferred tests so it's not entirely a pipe dream. I ran into a few issues which I worked around but...
|
I've moved my implementation across to using Promises instead of Deferreds as the basis. Here's a link to a repo that has my current work -- https://github.com/skaegi/promises-cancel. CancellablePromise.js (and Promise.js) in the repo above are now Promises/A+ 1.1 compliant as well as matching the "Promise Object" spec so it appears this sort of sub-classing is possible albeit requiring a fair bit of gymnastics. In addition to the two issues above another is...
Since I was now passing the wrapped object down to the superclass method this means I also had to do the identity check in my sub-class to ensure I was not resolving the promise in my sub-classes code. |
This is not final in any way, and as mentioned previously this is not urgent since
@@create
is the only way to make this relevant, but I wanted to get it out there.Drawing heavily on Allen's work on
Array
and so on, this series of commits uses a series of techniques to makePromise
subclasses Just Work(TM). The static methods will all return instances of the new type, and in fact will in most cases execute any special behavior for when you do something like:(stare at this example for long enough and I'm pretty sure it will make sense.)
Promise.cast
is a bit special since we want to ensure it always gives you back whatever type you call it on (e.g.InstrumentedPromise.cast(x)
always gives you anInstrumentedPromise
). The technique used there is a bit tricky and could be strengthened; I'm not a huge fan of it right now but we'll see.Example for @slightlyoff of passing in new capabilities to the resolver: