new Model(shapes, undefined) throws a TypeError

[postcasio]

the constructor for Model accepts an optional second argument, shader, to specify the shader. If you explicitly pass undefined for this argument, it throws TypeError: 'undefined' is not a Shader object. The C method only checks the number of arguments passed to determine if a shader was supplied, but it should also check if the argument is undefined.

This breaks simple code like:

const shader = FS.exists(shaderPath) ? loadShader(shaderPath) : undefined;
return new Model(shapes, shader);

[fatcerberus]

In fact if you look through pegasus.c, the entire API is like this. Background: Since JS has two null values (null and undefined) I don't like using undefined explicitly in my own code and treat it as strictly a garbage value, which is useful in the debugger to distinguish values that were explicitly set to null vs. ones unset due to a bug. So the rationale was that I wanted it to be an error if the user explicitly passed in garbage.

BUT, that being said--if I were to write the same function in JavaScript, I would almost certainly check for undefined to detect argument presence and not use arguments.length, so this should indeed be changed.

[fatcerberus]

@postcasio Workaround for your specific case:

const shader = FS.exists(shaderPath) ? loadShader(shaderPath) : Shader.Default;
return new Model(shapes, shader);

[rhuanjl]

You could compare against undefined AND null?

[fatcerberus]

No, because it's not valid to set no shader for a model, so null wouldn't be legal; the distinction I make is that undefined = not yet initialized, null = actually nothing.

The current implementation does the equivalent of this:

if (arguments.length >= 2) {
    if (!(arguments[1] instanceof Shader))
        throw TypeError("not a shader");
    this.shader = arguments[1];
}
else {
    this.shader = Shader.Default;
}

which fails for @postcasio's use case above. Normally I consider such failure to be desirable: when an undefined is observed at runtime, I like to have it cause an error--I won't typically check for it explicitly and just let it crash the program. If there's a case where a value is allowed to be unset, I'll check for null, but still not undefined. This is useful when running under a debugger to detect logic errors where a variable didn't get properly initialized.

In this case though, were I writing a function, I do normally check for undefined to detect missing arguments--not least because using arguments prevents optimizations--so the way the API is currently set up is wrong.

[rhuanjl]

Ah right - I see I thought you were implying that you only didn't check for undefined as checking for both null and undefined would be weird.

I see your point though - if someone passes undefined - is it because they didn't mean to pass an argument at all OR is it because they forgot to initialise something. Hmm I wonder what the JS library methods do for that, will have to check/I wonder if it's consistent.