Open hmallen99 opened 1 year ago
I think that making this change could make sense. Personally, I wouldn't write a function that can return a Promise sometimes and other times a plain value. In that case, having the error in place would alert me of a potential mistake I made. On the other hand, I could see how arrays like this could be created, and since the Promise library is used by a large number of people with many differing views, I think it makes sense to be more flexible.
Knowing that JS Promises handle this case differently than we do means that many people who use the library (yourself included, it seems!) might expect it to just work the same way, so eliminating any unnecessary surprises seems like a good thing.
There are two other functions where this error is emitted, Promise.race
and Promise.allSettled
. I checked how JS handles Promise.allSettled
and it reports plain values as "fulfilled". So, we should likely also follow suit and report them as "Resolved" rather than erroring.
For Promise.race
, my initial intuition is that we should probably keep the existing behavior we have today. We would just be resolving the value for the user without doing much work. But, I'd be interested to hear your thoughts on this.
Currently, Promise.all will give the error
Non-promise value passed into Promise.all at index i
if you attempt to pass in non-promise values: https://github.com/evaera/roblox-lua-promise/blob/0c095587f82ce37e3b249ae1d8665704f3824740/lib/init.lua#L491-L497.This is a bit different from the javascript Promise.all implementation, which will resolve non-promise values:
While javascript is not always the best place for inspiration, I think this is a convenient feature. You may have some data in a list that can be resolved synchronously, while you may have other data is resolved asynchronously.
For instance, you may want to fetch some data from the network, while other data may already be in a cache. It may not make sense to add the overhead of creating a Promise object for each cached item, which can be resolved synchronously, especially if you're looking up thousands of keys.
I think it would be great to align to the javascript implementation here, but let me know what you think! As far as implementation goes, you could add a check if the value is not a promise in the resolve loop, and add immediately call resolveOne here: https://github.com/evaera/roblox-lua-promise/blob/master/lib/init.lua#L546-L549
Thanks for taking the time to consider this proposal!