e-conomic/phantom-render-stream — pull request #45 by markstos (2014-07-28 18:27:02+00:00)
Title: Update default options handling to be consistent, easier to read.

Before, there were three ways that default options were being set:

opts.phantomFlags = opts.phantomFlags || [];
opts.maxErrors = typeof opts.maxErrors === 'number' ? opts.maxErrors : 3;
opts.timeout = opts.timeout !== false ? opts.timeout || 30000 : 0;

After the change, all options are set in a consistent way using the first
style.

The second style of checking that the input is a number is potentially
problematic, because a default is silently used if it is not. If
'maxErrors' should never be a number and it's important to check this, then
the best behavior would be to throw an exception so the developer finds out
right away that there's a problem, rather than being surprised or confused
later about the silent substition of a default value.

For now, we just pass through a value of the wrong type as we do with all the
options. This is more likely to lead to a useful diagnostic for the developer
than the silent substition of the default value, which would unexpectedly work
with a different value.

Regarding the handling of the "timeout" value, it appears to be complicated
way to express "opts.timeout || 30000". I tested the result of several value
including 'undefined', 'null, '0' and '1' got the same result with the simple
logic vs the complex logic in all cases.

Another inconsistency was that some defaults were set on "opts" while other
defaults were set only on "vars" that follows. Now all defaults are set
directly on 'opts' for consistency.

Finally, the defaultOpts are moved to their own object, where it's easy to
review them all in one place.