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

overload Promise.all #13

Open
hax opened this issue Mar 23, 2023 · 23 comments
Open

overload Promise.all #13

hax opened this issue Mar 23, 2023 · 23 comments

Comments

@hax
Copy link
Member

hax commented Mar 23, 2023

Consider currently Promise.all({}) just throw, practically we could overload Promise.all:

const {a, b, c} = await Promise.all({a: getA(), b: getB(), c: getC()})

Pros: No naming issue (Promise.allOwnProperties() is too lengthy)

Cons: Theoretically, people could add Object.prototype[Symbol.iterator] and change the behavior.

@ljharb
Copy link
Member

ljharb commented Mar 23, 2023

An additional con is that if they're passing a non-iterable right now it's almost certainly a bug, but this would mask it.

For example: Promise.all(getA(), getB(), getC()) is a likely bug where they forgot to wrap it in [ ], which currently throws - with this overload, it would just return an empty object because the a promise has no properties.

iow, this feels like a nonstarter to me.

@ajvincent
Copy link
Collaborator

Yeah, this is begging for trouble.

@ljharb ljharb closed this as not planned Won't fix, can't repro, duplicate, stale Mar 23, 2023
@hax
Copy link
Member Author

hax commented Mar 23, 2023

MM also suggested the same issue, should we reopen?

@ljharb
Copy link
Member

ljharb commented Mar 23, 2023

@erights' suggestion was a bit different: basically saying "if the first arg is not iterable and arguments.length > 1, throw", and then the fallback would be to accept the lone non-iterable object argument and return an object.

@ljharb ljharb reopened this Mar 23, 2023
@hax
Copy link
Member Author

hax commented Mar 23, 2023

Generally I like be strict on argument length, and it seems solve the misuse problem in this specific issue.

@erights
Copy link

erights commented Mar 24, 2023

Not just Promise.all but all four, overloaded exactly this way.

I favor that it operates on exactly the enumerable own properties, whether string-named or symbol-named, like ... and assign do.

Nice uniformity across the two overloads because, for a normal array, all the numeric indexed properties are also enumerable own.

@TomasHubelbauer
Copy link

Not just Promise.all but all four, overloaded exactly this way.

This is definitely the way to go when it comes to designing a nice API that is going to be a joy to use.

We already have a bunch of ugly names in JavaScript, and we even have a precedent for trying to fix them, like Object.prototype.hasOwnProperty being replaced by Object.hasOwn now. We don't need more ugly names.

The method should always take a single argument so that when someone forgets [ and] they get a runtime error and realize the mistake.

@Akxe
Copy link

Akxe commented Mar 24, 2023

An additional con is that if they're passing a non-iterable right now it's almost certainly a bug, but this would mask it.

For example: Promise.all(getA(), getB(), getC()) is a likely bug where they forgot to wrap it in [ ], which currently throws - with this overload, it would just return an empty object because the a promise has no properties.

iow, this feels like a nonstarter to me.

Pretty much every developer now a day uses some IDE. Given that the IDE will complain that the function got too many arguments.

I would say that in the modern day, this complaint is no longer valid.

@ljharb
Copy link
Member

ljharb commented Mar 24, 2023

I don't use an IDE, and neither does every developer. The complaint will forever be valid.

@Akxe
Copy link

Akxe commented Mar 24, 2023

I don't use an IDE, and neither does every developer.

That's why there is Pretty much every and not every... What do you use, then? (I am curious)

The complaint will forever be valid.

Both complaints will be forever valid. The point of this discussion is to determine if the benefit of putting an object to this function would ease more people than it would hurt.

Pros:

  • A separate name for awaiting Array and Object could confuse new users.
    • Promise.all(...) and Promise.ownProperties(...) (or Promise.fromEntries(Object.entries(...))) is not something a new user not understanding own properties could easily deduce.
    • Promise.all(...) should take all

Cons:

  • Users might put an object with defined Symbol.iterator, possibly throwing off the expected result
    • If using IDE, the result would be correctly deduced from the iterable type, thought caught, if not...
    • For some, this might even be a desired "side" effect
  • If decided that the number of arguments should not throw
    • People with IDE should once again be saved it
    • The others, I would assume they are knowledgeable enough to know what happened

@ljharb
Copy link
Member

ljharb commented Mar 24, 2023

I don't think a separate name is confusing to anyone; arguably, IDE autocomplete means it would be less confusing - that said, if we decide on a way to safely overload the existing mechanism, that's nice.

@acutmore
Copy link
Collaborator

Personally I very much like the idea of overloading the existing methods.

As mentioned there is the risk of Symbol.iterator being added or removed from an object which is also being passed to the overloaded Promise.all and this changing the outcome at a distance. I think this risk level is low because:

  • the primary use case for this overload is passing in an object literal, so the Symbol would need to be added to Object.prototype to go from the dictionary overload to the current iterable semantics.
  • The check for arguments.length === 1 reduces the risk when ...arr is used unintentionally, omitting the case where the iterable has a length of one.
  • There are already other APIs which have fallback semantics if the associated Symbol protocol is not present, such as Array.from or toString.

One hypothetical way to further reduce the risk would be to only accept iterables or objects with a prototype set to null. However I think the complexity, boilerplate, and restrictiveness of adding this limitation is not justified for the estimated level of risk.

It would be great to get as much feedback from developers with a diverse range of backgrounds on how their would find the overload vs separate dedicated APIs.

@Akxe
Copy link

Akxe commented Mar 24, 2023

Technically you could create an object with an iterator like so:

const obj = {
  [Symbol.iterator]: function* () {
    yield 1;
    yield 2;
    yield 3;
  },
};
await Promise.all(obj) // [1, 2, 3]

If a user does this, it is done by design... The risk, therefore, arising from this is minimal. However, this (or any similar) example should be documented in the proposal.

May I suggest a poll? (vote for both if you please)
🚀: overload
🎉: separate dedicated APIs

@ajvincent
Copy link
Collaborator

ajvincent commented Apr 16, 2023

After a few weeks, my passion for opposing this has cooled a bit. I care more about a workable solution than how that solution is implemented.

That said, I'm thinking also about type definitions from TypeScript, which gives me another concern. The current type definitions in TypeScript for Promise.all are:

// from es2015.iterable.d.ts
all<T>(values: Iterable<T | PromiseLike<T>>): Promise<Awaited<T>[]>;

// from es2015.promise.d.ts
all<T extends readonly unknown[] | []>(values: T): Promise<{ -readonly [P in keyof T]: Awaited<T[P]> }>;

https://github.com/microsoft/TypeScript/blob/main/src/lib/es2015.iterable.d.ts
https://github.com/microsoft/TypeScript/blob/main/src/lib/es2015.promise.d.ts

What would the types look like when overloaded?

@bakkot
Copy link

bakkot commented Apr 16, 2023

@ajvincent Presumably an extra overload along the lines of

all<T extends { [key: string]: unknown }>(obj: T): Promise<{ [P in keyof T]: Awaited<T[P]> }>

?

Seems to work fine in the playground.

@Akxe
Copy link

Akxe commented Apr 16, 2023

Since the array has -readonly I would assume this one would too. But yeah this overload is simple and correct.

@ctcpip
Copy link
Member

ctcpip commented Jul 12, 2023

if overloading, we would be overloading all of the Promise concurrency methods, right?

@ljharb
Copy link
Member

ljharb commented Jul 12, 2023

Both Promise.all and Promise.allSettled, I'd expect.

@ctcpip
Copy link
Member

ctcpip commented Jul 12, 2023

ah right, this wouldn't make much sense for any and race

@acutmore
Copy link
Collaborator

There could be an overload for them...

const result = await Promise.race({
  a: new Promise(() => {}), // never resolves - always loses the race
  b: Promise.resolve(42),
}); 

result; // { b: 42 }
"a" in result; // false
"b" in result; // true

However I agree that I don't think we should pursue this design because Promise.race does not currently return an array container type so this overload would make a more significant change to the return type by adding a container that isn't there when an iterable is passed as an argument.

@Akxe
Copy link

Akxe commented Jul 13, 2023

Promise.all and Promise.allSttled should return the container as resolved from the Symbol.Iterator or enumerable keys of given object (in this order).

Promise.race and Promise.any should accept iterable and object but return just the result of the promise.

We must be consistent with the current API.

@acutmore
Copy link
Collaborator

acutmore commented Jul 13, 2023

We must be consistent with the current API.

The consistency here is that the overload is available when the method returns a container.

To pass an object to Promise.{race, any} could still be done as Promise.{race, any}(Object.values(obj)).

@Akxe
Copy link

Akxe commented Jul 13, 2023

To pass an object to Promise.{race, any} could still be done as Promise.race(Object.values(obj)).

True the benefit added by overload of Promise.{race, any} is minimal. Let's not mess with more than is necessary.

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