Prevent invalid keys from being passed in options. - #348

@Rich-Harris ping.

@eventualbuddha Good idea. I see you're throwing an exception instead of returning a rejected Promise.

Since you're doing argument validation, throwing makes more sense to me. The reason I point this out is that the two other argument validation conditions return a rejected Promise instead. I think those should probably throw as well.

@Victorystick ah, good point. Those should be consistent, I agree. It might make more sense for the function to always return a promise, though. That way callers simply need to have a .catch( call chained as the sole way to handle errors. Making validateKeys return a promise would be a little strange, though, so perhaps it's best to wrap it somehow in the call to rollup?

Isn't there a semantic difference between a rejected Promise and a thrown Error? I agree that it's better to have a single way to catch errors, but these are two completely different kinds of errors, no?

Imagine a caller of the rollup function wants to handle errors. With the
way it's done now there are two kinds of error: immediately thrown errors
and rejected-promise errors. That means you need something like this:

try {
rollup(…)
.then(…)
.catch(/* rejected promise handler */);
} catch (err) {
/* immediately thrown handler */
}

Whereas if we always return a promise you can drop the surrounding
try/catch.
On Tue, Dec 8, 2015 at 8:14 AM Oskar Segersvärd notifications@github.com
wrote:

Isn't there a semantic difference between a rejected Promise and a thrown
Error? I agree that it's better to have a single way to catch errors, but
these are two completely different kinds of errors, no?

—
Reply to this email directly or view it on GitHub
#348 (comment).

@eventualbuddha @Victorystick If we use promises in rollup then we should always reject and error with it. As an alternative you can wrap all in Promise and throw an error there

function rollup() {
  return Promise.resolve().then(function () {
    throw Error('Some error');
  });
}

This will be the same as returning Promise.reject();

FWIW, this is the same motivation why ES'15 Promise functions do not throw but return rejected promises.

Promise.all(42); // rejected Promise

(V8 gets this wrong #4603)

@arv Doesn't matter in this case

I'll redo it with the semantics discussed.

Done. Added a test for it as well.

I took the liberty of replacing the for...of loop with a while – with our current build setup, for...of isn't actually transpiled. We could add that setting to the .babelrc, but it doesn't transpile well, so I tend to include it in the 'ES6 features to avoid' category.