-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prevent invalid keys from being passed in options. #348
Conversation
@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 |
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 try {
rollup(…)
.then(…)
.catch(/* rejected promise handler */);
} catch (err) {
/* immediately thrown handler */
} Whereas if we always return a promise you can drop the surrounding
|
@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 |
Prevent invalid keys from being passed in options.
These are all the keys needed to make the test pass, but I may have missed something.