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

Additional complexity you could remove from Promises #5

Open
domenic opened this issue May 21, 2020 · 17 comments
Open

Additional complexity you could remove from Promises #5

domenic opened this issue May 21, 2020 · 17 comments

Comments

@domenic
Copy link
Member

domenic commented May 21, 2020

Adding subclassing support to promises, with all its attendant complexity, is one of my greatest regrets. I'm very excited about the possibility of fixing that.

This proposal doesn't go as far as it could in removing the complexity that I added. Here are some additional things that could be done:

  • PromiseResolve could stop checking the .constructor property, instead just using IsPromise alone.
  • Promise.prototype.catch and Promise.prototype.finally could add brand checks and no longer delegate to this.then. For .finally especially, this could result in a dramatic simplification, eliminating the majority of the logic in the "Then Finally Functions".
  • Everything related to NewPromiseCapability() could be dramatically simplified:
    • It would no longer need to take a constructor, and thus all its call sites would no longer need to lookup this.constructor/this.constructor[@@species].
    • It would no longer need to actually call the constructor and capturing the resulting resolving functions, since the constructor is always %Promise%. You could just create the new %Promise% instance directly. (So, GetCapabilitiesExecutor functions goes away.)
    • It would no longer need to check if the resulting resolving functions are functions.
  • In fact you could probably the PromiseCapability concept entirely, instead just directly using a new ResolvePromise and the existing RejectPromise throughout the spec.
    • Because ResolvePromise/RejectPromise don't throw, a lot of error checking and propagation for misbehaved resolve/reject functions could be removed.
    • I believe some of the [[AlreadyResolved]] shenanigans could also disappear, as they were mostly in place to deal with misbehaved subclasses? This isn't as obvious though.
  • The static methods can similarly be simplified
    • Because they no longer need to delegate to .then, a lot of error checking can be removed. (Including, I think, [[AlreadyCalled]].)
    • They no longer need to delegate to this.resolve.

More insight might be gathered by looking at some of the commits from the original ES6 promises spec draft that introduced all this subclassing complexity:

@syg
Copy link
Collaborator

syg commented May 21, 2020

Importantly, do you feel removal for such delegation-by-property-lookup is web compatible for Promises? Is your intuition that such delegation is exceedingly rare, like in the case of similar machinery for RegExps?

@domenic
Copy link
Member Author

domenic commented May 21, 2020

Yes, that's my intuition. Although I can find a few promise subclass packages on npm, I can't see any signs of life on them, and I've never heard of any promises being subclassed in the wild. (Also, of those npm packages, none of them seem to override then or resolve at first glance.) I've also certainly never heard of anyone tearing off Promise.prototype.finally and applying it to their own then-containing class in the hopes that it will call .then and this.constructor[Symbol.species] in some useful way.

@Ginden
Copy link

Ginden commented Jun 8, 2020

Promise.prototype.catch and Promise.prototype.finally could add brand checks and no longer delegate to this.then.

@domenic I personally answered question how to abuse .then on StackOverflow. Probably some people depend on this behavior.

@ljharb
Copy link
Member

ljharb commented Jun 8, 2020

fwiw, I'm pretty certain none of this complexity removal would break any Promise shims - either the entire functionality or finally alone - but, it may cause runtime feature tests to fail, which may cause Promise to be unnecessarily shimmed (which wouldn't break anything, but would be unfortunate).

@dead-claudia
Copy link

dead-claudia commented Jun 19, 2020

I do want to note that Mithril does rely on .then and .constructor hacking, with the constructor set to a very simple proxy value. (What it does is wrap it to schedule a global redraw after it resolves, though this can be disabled entirely via background: true.) It's behavior I plan to eventually remove, as it's only kind of useful in very limited circumstances, and it doesn't nudge people to properly handle errors like they should. If this proposal reaches stage 2 and .catch and .finally are slated to be removed, I'll immediately deprecate that behavior, pointing to this proposal.

I didn't put that particular aspect in the API - the framework's original author did. Problem is, a large portion of the community likes it, and I need a good alternative to exist or my hand forced somehow (like via this proposal) before I can finally remove it.

@Jack-Works
Copy link
Member

I just used subclassing Promise days ago like this:

class X extends Promise {
    constructor(e) { super(e); super.catch(console.warn); }
    static get [Symbol.species]() { return Promise }
}

@ljharb
Copy link
Member

ljharb commented Jun 19, 2020

You could replace all of that with a constructor that does return new Promise(e).catch(console.warn), fwiw

@mhofman
Copy link
Member

mhofman commented Jun 19, 2020

Not quite though.
const p = new Promise(e); p.catch(console.warn); return p

Returning the chained promise would swallow errors.

@dead-claudia
Copy link

@Jack-Works In either case, you could still just use an ES5 function to accomplish almost the same effect:

function X(...args) {
	const p = new Promise(...args)
	p.catch(console.warn)
	return p
}

As for how each type would affect you if you wanted to update your code and you don't care about p instanceof X:

  • Type 1: you're not broken at all.
  • Type 1-2: change your static get [Symbol.species]() { ... } to get constructor() { ... }
  • Type 1-3: you're not broken at all.
  • Type 1-4: what's going on today.

@littledan
Copy link
Member

I'd also note that the semantics proposed here would leave @Jack-Works 's code working exactly as it does today. The only thing it would change is what happens if you don't override [Symbol.species] to point to Promise.

@Jack-Works
Copy link
Member

I'd also note that the semantics proposed here would leave @Jack-Works 's code working exactly as it does today. The only thing it would change is what happens if you don't override [Symbol.species] to point to Promise.

And without specifying species, it becomes a deadloop

@littledan
Copy link
Member

To clarify, I'm saying, if we make it so that Promise.prototype.then always returns a Promise, rather than using SpeciesConstructor (as proposed in the original post on this thread), and we left Symbol.species in place, just ignored (as proposed in the README in general), then your code will continue to work--it just won't call Symbol.species.

@zloirock
Copy link

zloirock commented May 5, 2021

The same as #23.

The generic nature of Promise methods, NewPromiseCapability, PromiseResolve logic, etc are actively used in polyfills, at least in core-js.

Even with a polyfilled Promise constructor, they try to use native methods. As you could see in #23, with this proposal Promise will be polyfilled, even without it in the pure version of core-js Promise wrapped for preventing global pollution. Some methods have detection of generic nature and will be forcibly polyfilled, but the most (at least modern) - not, so they just will stop work.

@dead-claudia
Copy link

@zloirock Not sure that polyfill would break the non-generic detection or the species detection - it'd just silently start adding it back in every modern browser. Of course that'd need fixed in core-js, but devs would have to update core-js before they'd notice anything different.

@zloirock
Copy link

zloirock commented May 6, 2021

@isiahmeadows seems you didn't read #23. Yes, you could say that it will not break the most methods - they will not stop work - they just will be tens and hundreds of times slower - I think that it could be called "breaking". But some cases will be completely broken - for example, Promise-related.

I'm too lazy to come up with complex cases related directly to this issue, so the most simple. For example, since Promise will be polyfilled, but methods like Promise.any not, Promise.any(promises) instanceof Promise // -> false.

Do you really think that you could say all developers immediately update their dependencies? -) I already wrote that core-js is used on most websites. core-js@2 was deprecated (with deprecation message and explanation of problems) 2 years ago - but at this moment it's more than 40% of used core-js versions. 2 of the top 10 internet websites use core-js@1 - 6 years old.

@dead-claudia
Copy link

@isiahmeadows seems you didn't read #23. Yes, you could say that it will not break the most methods - they will not stop work - they just will be tens and hundreds of times slower - I think that it could be called "breaking".

Okay, I meant functionally non-breaking, not "broken" because it's slow. (That's of course a whole separate issue, and if that's the case, I'd love to stand corrected.)

I did look at the source and now I see some risk of breakage, specifically when running in V8: https://github.com/zloirock/core-js/blob/49f13bfbfb4a48fe82d178df58fc943103e24997/packages/core-js/modules/es.promise.js#L65-L68 I'll stand corrected on this criticism now - that bit of code gives me a little bit of pause.

But some cases will be completely broken - for example, Promise-related.

I'm too lazy to come up with complex cases related directly to this issue, so the most simple. For example, since Promise will be polyfilled, but methods like Promise.any not, Promise.any(promises) instanceof Promise // -> false.

Do you have a link to the relevant source? It'd be very strange for Promise.any to return anything but Promise, and I'm struggling to come up with how that might happen, especially if I look at the relevant source (short @@species hacking, though that's part of my point).

Do you really think that you could say all developers immediately update their dependencies? -) I already wrote that core-js is used on most websites. core-js@2 was deprecated (with deprecation message and explanation of problems) 2 years ago - but at this moment it's more than 40% of used core-js versions. 2 of the top 10 internet websites use core-js@1 - 6 years old.

I'm not saying developers would immediately update their dependencies - I was in fact trying to say I didn't think they would need to.

@zloirock
Copy link

zloirock commented May 7, 2021

I did look at the source and now I see some risk of breakage, specifically when running in V8

Seems you didn't understand the source of this feature detection. In the modern V8 it will not be replaced because of this line so in V8 the number of problem cases reduced. But only in V8 and with recent core-js versions (it was added in minor core-js@3 versions).

Do you have a link to the relevant source?

It's too many places - see the source code of feature detections and export logic, the explanation why - above. Some methods will work in the global core-js version, but not in the pure.

especially if I look at the relevant source (short @@species hacking, though that's part of my point).

In the polyfill, but not in the native method which will be used - it does not include any feature detection.

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