tc39 / proposal-await-dictionary

A proposal to add Promise.ownProperties(), Promise.fromEntries() to ECMAScript
MIT License
86 stars 4 forks source link

A thought about API naming #7

Open rwaldron opened 3 years ago

rwaldron commented 3 years ago

Since both APIs are creating something from an initial thing, it might make sense for consistency to name them:

WDYT?

ljharb commented 3 years ago

"Promise fromEntries" suggests to me that it makes a promise, since Object.fromEntries makes an object (and Array.from makes an array, etc)

ajvincent commented 3 years ago

I'm inclined against this idea but very weakly. Anything hung off Promise, I would expect, to make a promise - that's not my objection. If we are going to introduce other methods analogous to allSettled, race, etc. (#4), having that extra "from" prepended would just make the method names longer.

It wouldn't take much convincing to change my mind. It's a good suggestion.

jamiebuilds commented 1 year ago

I would like to see it as a parallel to Promise.all() as Promise.allEntries() and you could append/interject Settled if an equivalent to Promise.allSettled() is wanted in the future.

Haroenv commented 1 year ago

I guess this has a reason, but why not Promise.all({…})? For me the array and object properties can probably both be modelled as an object, as you can destructure both ways. Is it because passing an object throws now?

ljharb commented 1 year ago

@Haroenv Promise.all accepts an iterable - any objects can be iterable - and treating non-iterable objects as "not a bug" is likely to be a footgun.

lubieowoce commented 1 year ago

Not sure if this is the right place to put this, but i thought i'd my 2 cents on the ownProperties name.

TBH, I'm not a fan of it! it feels like the emphasis on "own properties" is distracting from the point. i'd imagine 99% of the usages will be an object literal of promises, a case where most people don't think about prototype chains, because they won't come up in any significant way. but now, since the name highlights ownProperties, i'm instantly led to think about non-ownProperties, even though it's unlikely to matter at all! in a sense it's too precise.

(this reminds me of the old "don't think about elephants" mind trick -- "quick, try not to think about prototype chains" ;) ).

do we have a good name for "just a plain key-value object of stuff" that'd work here? record/map are both a bit overloaded, and dictionary is kinda unwieldy... maybe focus on the "naming" aspect instead, like Promise.allNamed, allKeyed, allByKey or something along those lines? i don't love those particular ones too much, but i think i'd much prefer that general direction

ljharb commented 1 year ago

we already have Reflect.ownKeys and Object.getOwnPropertyDescriptor - but, to be fair, we also have Object.assign, and Object.entries and friends, which also only deal with own properties.

lubieowoce commented 1 year ago

Right, and in Reflect.ownKeys the precise language makes sense -- w/ Reflect, it's useful to tie it to the precise term from the language spec. But i don't think this proposal benefits from that precision at all. so IMHO the Object.entries school of naming would make more sense here.

the more i think about it, the more something like allByKey makes sense -- it draws an analogy with Object.keys() (and it'll use the same subset of properties AFAIU) while also not accidentally suggesting it'll touch non-own properties like allProps does. and the scheme extends to the other promise methods pretty nicely -- allSettledByKey, anyByKey, raceByKey all sound quite alright for me.

ljharb commented 1 year ago

@lubieowoce the word "all" explicitly implies "own and inherited", and "any" (due to anySettled) implies that it only affect one key.

lubieowoce commented 1 year ago

the word "all" explicitly implies "own and inherited"

we clearly have different perspectives here, because it reads completely differently for me 😃

for me, the "own" is implied, because that feels like the only sensible behavior -- i can't see myself ever wanting to include inherited properties here. so the topic of own/inherited doesn't even come to mind -- "all" evokes Promise.all, and that's it.

ajvincent commented 1 year ago

I'm with @ljharb on this one. Originally, I had pitched allProperties as the name, but we quickly pivoted to ownProperties because of the ambiguity in dealing with prototype chains.

Ambiguity is precisely what we don't want. Keep in mind this is a proposal to change the ECMAScript standards, and thus, change the JavaScript language forever. Forever tends to be a lot longer than anyone thinks. (Error.prototype.stack is the example that comes to mind, because I inspired it.)

As a counter-example, consider this (deliberately buggy) code:

let A_res, A_rej;

function A() {}

A.prototype.firstPromise = new Promise((res, rej) => {
  A_res = res;
  A_rej = rej;
});

let B_res, B_rej;
function B() {}
B.prototype = new A;

B.prototype.secondPromise = new Promise((res, rej) => {
  B_res = res;
  B_rej = rej;
});

let C_res, C_rej;
function C() {}
C.prototype = new A;

C.prototype.thirdPromise = new Promise((res, rej) => {
  C_res = res;
  C_rej = rej;
});

// Now, let's use these.
const d = new B;
const e = new C;

d.firstPromise === e.firstPromise; // true

// some setTimeouts or whatever to schedule resolving or rejecting the promises
await Promise.allProperties(d); 
// under the suggestion, this settles e.firstPromise, which is a side-effect!

That's not very nice for developers to run into. But it's fundamentally how JavaScript works in looking up properties. (Reflect.get versus Reflect.getOwnPropertyDescriptor.) Walking the prototype chain will cause this kind of unexpected behavior for developers, with unexpected and/or inconsistent results.

zloirock commented 1 year ago

Low-level reflection methods have a long descriptive name, like Object.getOwnPropertyDescriptors, and methods for everyday usage have a short name, like Object.hasOwn.

Maybe makes sense to follow this pattern and use something like Promise.allOwn?

lubieowoce commented 1 year ago

because of the ambiguity in dealing with prototype chains.

As a counter-example, consider this (deliberately buggy) code:

i've said this already, but I'd expect 99% of the usages of this method to be on an object literal of promises, so this argument feels a bit contrived to me. and i haven't ever heard anyone complain about the fact that Object.entries is "too ambiguous w.r.t treatment of prototype chains".

also, TBH, i'm just not looking forward to having to explain prototypical inheritance to a junior dev who asks me why there's an "own" in the name, when all they wanted to do is a Promise.all with some keys attached. if we do go with ownProps as the name, i fully expect 10000 questions from new devs about "why does this straightforward API have such a weird name?"

so all in all, i really strongly feel that a user-friendlier name would be better. and by that i mean a name that doesn't make me think about prototypes, because for 99% of the cases they're not gonna matter here. i think everyone would tacitly assume that it only works with own properties anyway, because people just don't (consciously) use prototypes much. but that's just me :)

One last comparison with another rarely used feature -- holey arrays. i just wondered if [1,,3].filter((x) => true) will include holes or not, and had to go check. that could well be considered an ambiguity! but IMHO it's not something that needs clarification in filter's name, because it just doesn't come up often enough to warrant that.

jamiebuilds commented 1 year ago

In this context, even mentioning "own" is actually more confusing.

I can't even imagine a real-world scenario where you'd actually want to pass an object with "non-own" promises on the prototype, where you'd need the method clarified for you that it only operates on "own".

I just went through the top ~100 examples of Promise.props() (Bluebird) and Promise.hash() (RSVP) examples on Sourcegraph and they all fell into two buckets:

// A: Object literal
await BluebirdPromise.props({ one, two })

// B: Built object with assigned props
let tasks = {}
tasks.one = doSomething()
if (cond) tasks.two = doSomethingElse()
await BluebirdPromise.props(tasks)

So mentioning "own" would force people to think about a concept they could otherwise totally ignore.

ljharb commented 1 year ago

I'm fine omitting "own", but "all" forces them to think about it too and means inherited also, so neither word seems advisable.

bakkot commented 1 year ago

"all" does not mean "inherited also", to my eye, and it no more forces people to think about inheritance than Promise.all does.

But even for people who think "all" means "inherited", they will only see a surprising result if there's a property on the prototype, or a non-enumerable own property, which holds a promise. Neither situation is likely to actually come up, and it will still give them the thing they're expecting except in those situations. It's fine.

ljharb commented 1 year ago

True, i meant in the context of "properties" :-) i also agree it's unlikely to show up, but I think if the argument against "own" is "you have to think about ownness vs inherited" then that applies also to "all".

Remember also tho, it's not just promises - it's everything, because Promise.all wraps every item in Promise.resolve().

bakkot commented 1 year ago

Remember also tho, it's not just promises - it's everything

Right, but for these hypothetical people who thinks that "all" means "also inherited", the extra properties they are imagining they'd get from the prototype wouldn't actually cause them problems except in the specific case that the properties on the prototype held promises and they wanted to await those values.

Outside of that unusual case, such a person going to look at Promise.allProps(obj) and think "oh there might be some extra crap from the prototype, but since none of those things are promises they're just going to be wrapped in Promise.resolve and immediately settle, and then I can just throw those values away". So it's not going to stop them from using it for its intended purposes.

The only downside of having that incorrect model are a.) in the case that this person actually does have a Promise-valued property on the prototype, which I am comfortable calling rare enough that it's not worth worrying about or b.) in the case that this person specifically wants to avoid having any extra properties on the result, in which case they might not use this method even though it would work for them, which, whatever.