Promise.all should not error for non-promise input values

[hmallen99]

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:

roblox-lua-promise/lib/init.lua

Lines 491 to 497 in 0c09558

	-- We need to check that each value is a promise here so that we can produce
	-- a proper error rather than a rejected promise with our error.
	for i, promise in pairs(promises) do
	if not Promise.is(promise) then
	error(string.format(ERROR_NON_PROMISE_IN_LIST, "Promise.all", tostring(i)), 3)
	end
	end

.

This is a bit different from the javascript Promise.all implementation, which will resolve non-promise values:

const promise1 = Promise.resolve(3);
const promise2 = 42;
const promise3 = new Promise((resolve, reject) => {
  setTimeout(resolve, 100, 'foo');
});

Promise.all([promise1, promise2, promise3]).then((values) => {
  console.log(values);
});
// Expected output: Array [3, 42, "foo"]

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.

local cache = {
    a = 5
}

local function fetch(key)
    if cache[key] then
        return cache[key]
    end

    return networkFetch(key):andThen(function(result)
        cache[key] = result
        return result
    end)
end

local keys = { "a", "b", "c" }
local results = map(keys, fetch)

Promise.all(results):andThen(...)

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!

[evaera]

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.