Skip to content
This repository was archived by the owner on Jan 26, 2022. It is now read-only.

Explicit species constructor logic unnecessary? #48

Closed
wants to merge 1 commit into from

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Feb 12, 2018

In order to be tolerant of Promise subclassing, polyfill implementations of Promise.prototype.finally typically implement logic to obtain the species constructor C, which is then used to call C.resolve(onFinally()) or new C(resolve => resolve(onFinally()) to ensure the resolved promise is instanceof the custom subclass.

However, a polyfill implementation of Promise.prototype.finally that does not need to call resolve explicitly could potentially avoid this extra effort to obtain the appropriate species constructor. If this new implementation is truly equivalent (modulo any fixable problems that I may have overlooked), then spec-compliant polyfills would no longer incur a penalty for following the spec exactly, compared to naive implementations that ignore these details for the sake of simplicity.

This pull request demonstrates one such implementation. Here's the essential logic:

var threw = false;
var result;
return Promise.prototype.then.call(
  this,
  function (value) {
    result = value;
    return onFinally();
  },
  function (error) {
    threw = true;
    result = error;
    return onFinally();
  }
).then(function () {
  if (threw) throw result;
  return result;
});

The trick is to return the result of onFinally() from the .then callback functions, which automatically resolves the return value to an instance of the current Promise subclass, using whatever resolution logic the subclass implements. The final .then will be invoked against an instance of the subclass, allowing the subclass to define the behavior of .then and thus the final result of the .finally method.

Note that a spec-compliant implementation of Promise.prototype.then will almost certainly still need to obtain the species constructor in its own implementation. However, it seems desirable to keep that logic out of Promise.prototype.finally if possible.

After considering this implementation, if you still believe the explicit species constructor logic is necessary, I would encourage you to write a test that demonstrates the difference.

If there are no objections to this new implementation, this repository seems like the appropriate place to provide updated guidance to other implementations. In other words, if this PR is merged, I will gladly submit PRs to other implementations, referring back to this PR.

In order to be tolerant of `Promise` subclassing, polyfill implementations
of `Promise.prototype.finally` typically implement logic to obtain the
species constructor `C`, which is then used to call
`C.resolve(onFinally())` or `new C(resolve => resolve(onFinally())` to
ensure the resolved promise is `instanceof` the custom subclass.

However, a polyfill implementation of `Promise.prototype.finally` that
does not need to call `resolve` explicitly could potentially avoid this
extra effort to obtain the appropriate species constructor. If this new
implementation is truly equivalent (modulo any fixable problems that I may
have overlooked), then spec-compliant polyfills would no longer incur a
penalty for following the spec exactly, compared to naive implementations
that ignore these details for the sake of simplicity.

This pull request demonstrates one such implementation. Here's the
essential logic of this approach:

```js
var threw = false;
var result;
return Promise.prototype.then.call(
  this,
  function (value) {
    result = value;
    return onFinally();
  },
  function (error) {
    threw = true;
    result = error;
    return onFinally();
  }
).then(function () {
  if (threw) throw result;
  return result;
});
```

The trick is to return the result of `onFinally()` from the `.then`
callback functions, which automatically resolves the return value to an
instance of the current `Promise` subclass, using whatever resolution
logic the subclass implements. The final `.then` will be invoked against
an instance of the subclass, allowing the subclass to define the behavior
of `.then` and thus the final result of the `.finally` method.

Note that a spec-compliant implementation of `Promise.prototype.then` will
almost certainly still need to obtain the species constructor in its own
implementation. However, it seems desirable to keep that logic out of
`Promise.prototype.finally` if possible.

After considering this implementation, if you still believe the explicit
species constructor logic is necessary, I would encourage you to write a
test that demonstrates the difference.

If there are no objections to this new implementation, this repository
seems like the appropriate place to provide updated guidance to other
implementations. In other words, if this PR is merged, I will gladly
submit PRs to other implementations, referring back to this PR.
@ljharb
Copy link
Member

ljharb commented Feb 12, 2018

This proposal is stage 4 and has already landed in the spec; this repository is now effectively archived.

That said, the polyfill code here (and in all proposal repos) is merely a demonstration, not ever intended for actual use; if you could show how this could work on https://npmjs.com/promise.prototype.finally (the closest thing there is to an official polyfill for this feature), combined with showing what the altered spec text in ecma262 would look like, it might be worth considering?

@benjamn
Copy link
Member Author

benjamn commented Feb 23, 2018

Related discussion on es-discuss today: https://esdiscuss.org/topic/promise-finally

@shixianqin

This comment has been minimized.

@ljharb

This comment has been minimized.

@ljharb
Copy link
Member

ljharb commented Jul 24, 2019

Given that this is stage 4, further changes should be posted on the main spec.

@ljharb ljharb closed this Jul 24, 2019
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.

3 participants