[Docs] Interesting, we don't have a standard for whether we refer to Promises with an upper-case P or lower-case p... Outside the scope of this PR, maybe it'd be a good followup to file an issue about standardizing this in docs?

Not a blocker or request for changes 😄 just ruminating.

[Question] Is there a use case for adding an option to allow users to provide an array/tuple where only some of the elements are thenables? LikePromise.all([callAsyncThing(), "other", callAsyncThing()])?

I can't think of one 🤔 so maybe this is a non-actionable comment?

Btw, sorry if I missed this in previous discussion - I looked through and couldn't find anything.

This is the main case I'm specifically looking to lint, and is the most common case where mistakes are made. There is no advantage to doing this from the perspective of control flow (i.e., how long the returned promise takes to fulfill), however, it does change the return value. However, whenever you really do want to provide a non-Thenable to change the control flow, you can always wrap it inPromise.resolve to get exactly the same behavior while making it way more clear how the aggregator will behave. This is still true even when considering if the returned promise is fulfilled synchronously vs asynchronously: these promise aggregators only fulfill synchronously when passed an empty array.

I would argue especially for all of these promise aggregators, the most important thing is making the control flow clear. Any non-Thenable value passed to a promise aggregator will be treated asPromise.resolve(value) according to the spec; see e.g.,PerformPromiseAll. Thus given that a value in the list is known to be non-Thenable, there is a static conversion available that will make the control flow easier to understand, or you can be explicit and wrap the value in Promise.resolve, which jumps out at the reader and thus also makes the control flow easier to understand.

Given:

declareconstaPromise:Promise<number>;declareconstbPromise:Promise<number;declareconstcNumber:number;

// allPromise.all([aPromise,bPromise,cNumber])// equivalent toPromise.all([aPromise,bPromise,Promise.resolve(cNumber)])// which resolves in the same amount of time asPromise.all([aPromise,bPromise])// previously working, now causes lint errorconst[a,b,c]=awaitPromise.all([aPromise,bPromise,cNumber]);// suggested changeconst[a,b]=awaitPromise.all([aPromise,bPromise]);constc=cNumber;

// allSettledPromise.allSettled([aPromise,bPromise,cNumber]);// equivalent toPromise.allSettled([aPromise,bPromise,Promise.resolve(cNumber)]);// resolves in the same time asPromise.allSettled([aPromise,bPromise]);// previously working, now causes lint errorconst[aPromise2,bPromise2,cPromise]=awaitPromise.allSettled([aPromise,bPromise,cNumber]);// suggested changeconst[aPromise2,bPromise2]=awaitPromise.allSettled([aPromise,bPromise]);constcPromise=Promise.resolve(cNumber);

// raceawaitPromise.race([aPromise,bPromise,cNumber]);// equivalent toawaitPromise.race([aPromise,bPromise,Promise.resolve(cNumber)]);// always resolves immediately (but never synchronously, as Promise.race is never synchronous)// previously working, now causes lint error// this is a if a is immediately resolved, b if b is immediately resolved, otherwise cconstaOrBOrC=awaitPromise.race([aPromise,bPromise,cNumber]);// suggested change if you really did want the above behavior (most of the time you do NOT)constaOrBOrC=awaitPromise.race([aPromise,bPromise,Promise.resolve(cNumber)]);// suggested change if you actually intended to wait for something to happenconstaOrB=awaitPromise.race([aPromise,bPromise])// note that most of the time this comes up, this is whats actually happening:declareconstaPromise:Promise<number>;declareconstbPromise:Promise<number>;declareconstcWrappedPromise:{promise:Promise<number>};// lint error that catches a real subtle bugconstaOrBOrC=awaitPromise.race([aPromise,bPromise,cWrappedPromise]);// correct codeconstaOrBOrC=awaitPromise.race([aPromise,bPromise,cWrappedPromise.promise]);

// any is the same as race for the above purposes, the difference is how rejected values are handled

[Non-Actionable] I realized a situation where this option would be helpful! [playground link]

declarefunctionfetch(url:string):Promise<object>;functionexample(){returnPromise.all([fetch("/"),"in the middle",fetch("/")]);}

If I was the developer here usingPromise.all directly to avoid having to make & manage a[object, string, object] tuple, I'd be annoyed at the rule asking me to explicitlyPromise.resolve("in the middle"). There's no benefit in this situation beyond consistency - at the cost of simplicity.

But, I get what you're saying that this is really the main point of the rule. I think it's fine for now to leave as-is. Since it's only enabled in the strict config, it should just show up for users who have opted into the more strict opinions.

Please do add an option that allows mixed arrays. I often parallel a bunch of tasks and sometimes tasks shift between being sync and async (for example, because it no longer dynamically imports a library, or it no longer reads a config file). I really don't want to break the symmetry in this case.

In that case, +1ing Josh's request 🙂

[Testing] Missing unit test coverage for this and a few other lines. If it's a case where weknow the value is definitely not nullish (e.g. an array type having a numeric index) then a! is reasonable.

[Refactor] Similar to theisBuiltinSymbolLike comment, this is only ever used with aSet<string>. I think expanding the use case to predicates is just a bit more complexity than we need.

this logic (ifisTypeAnyType(sometype) || isTypeUnknownType(sometype) ortsutils.isThenableType(checker, originalNode, sometype)) is used three times in this rule. Maybe we should move it to the function to avoid duplicated code?

Hmm; I refactored around a bit, though I still have 3 calls like this. However, I don't think something like this would add anything besides some mental overhead:

functionisTypeAnyOrUnknownType(type:ts.Type):boolean{returnisTypeAnyType(type)||isTypeUnknownType(type);}

For this to be useful from my current perspective there would need to be a more salient name for the method that is a more useful concept thanchecking for any or unknown and makes the implementation obvious (since it's so short). I looked elsewhere around the codebase and any types are not universally treated the same as unknown types, e.g., I could imagine a flag on this check later, perhaps two flags (one for any and one for unknown), and depending on exactly how those flags work it might make sense to break it out then, but more likely even if you made those flags it would still not be helpful to have a function like this given you would want different errors each which explanations of the flag to use to disable that feature, ect.