Callback arguments are not always optional

[irahopkinson]

It seems the callback arguments are only optional when the return type is string. For anything else they are not always optional. For example, the following all work as expected:

string resultStr = await StaticNodeJSService
    .InvokeFromStringAsync<string>("module.exports = (callback, message) => callback(null, message);", args: new[] { "success" });
resultStr = await StaticNodeJSService
    .InvokeFromStringAsync<string>("module.exports = (callback, message) => callback(undefined, message);", args: new[] { "success" });
resultStr = await StaticNodeJSService
    .InvokeFromStringAsync<string>("module.exports = (callback) => callback(undefined, 'success');");
resultStr = await StaticNodeJSService
    .InvokeFromStringAsync<string>("module.exports = (callback) => callback();");
int resultNum = await StaticNodeJSService
    .InvokeFromStringAsync<int>("module.exports = (callback) => callback(undefined, 7);");
object resultObj = await StaticNodeJSService
    .InvokeFromStringAsync<object>("module.exports = (callback) => callback(undefined, { result: 'success' });");
resultObj = await StaticNodeJSService
    .InvokeFromStringAsync<object>("module.exports = (callback) => callback(undefined, {});");

And the following throw the expected exception:

resultStr = await StaticNodeJSService
    .InvokeFromStringAsync<string>("module.exports = (callback) => callback(new Error('Expected exception.'));");
resultNum = await StaticNodeJSService
    .InvokeFromStringAsync<int>("module.exports = (callback) => callback(new Error('Expected exception.'));");
resultObj = await StaticNodeJSService
    .InvokeFromStringAsync<object>("module.exports = (callback) => callback(new Error('Expected exception.'));");

However, the following I would expect to work:

resultNum = await StaticNodeJSService
    .InvokeFromStringAsync<int>("module.exports = (callback) => callback();");
resultNum = await StaticNodeJSService
    .InvokeFromStringAsync<int>("module.exports = (callback) => callback(undefined, NaN);");
resultObj = await StaticNodeJSService
    .InvokeFromStringAsync<object>("module.exports = (callback) => callback();");

But they throw this exception (each have different inner exceptions):

Exception has occurred: CLR/System.Text.Json.JsonException
Exception thrown: 'System.Text.Json.JsonException' in System.Private.CoreLib.dll: 'The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true. Path: $ | LineNumber: 0 | BytePositionInLine: 0.'
 Inner exceptions found, see $exception in variables window for more details.

I would preferred a fix so that they work but at the least, the documentation could be changed to reflect reality, particularly section https://github.com/JeringTech/Javascript.NodeJS#function-with-callback-parameter.

[JeremyTCD]

Hey you're right, there are some issues here.

1. We need a clearer exception message for when a return value is invalid. E.g if we're expecting an int and the returned value is "randomstring", an InvocationException should be thrown with message:

   Expected return value of type int, invoked script returned "randomstring".

   It is indeed hard to pinpoint the problem from current JSONExceptions.

2. Every invoke method has a corresponding overload with no generic parameter for use when you don't expect a return value. We need to document these functions in the ReadMe's API section. Example method:

   // Returns nothing
   await StaticNodeJSService.InvokeFromStringAsync("module.exports = (callback) => callback();");

3. ReadMe should state that the result parameter is not optional if a return value is expected.

Does that cover everything? Let me know if you have further suggestions.

[irahopkinson]

Well what about when we do expect something but its not given. In JS a number can be undefined or NaN and an object can be undefined so the last 3 examples I gave that don't work now, I would expect to work without throwing an exception. Not sure you covered that in your 3 responses.

[JeremyTCD]

Good point. I considered returning default values in such situations but it's tricky:

This library doesn't manually parse values returned by scripts. Apart from strings and streams, it serializes to JSON on the Node.js side and deserializes from JSON on the C# side. This goes for ints, doubles, objects etc. As stated in the ReadMe:

It (the return value) must be a JSON-serializable type, a string, or a stream.Readable.

We could return the default value when a JSONException is thrown: e.g 0 when a script returns "NaN":

// Returns 0
resultNum = await StaticNodeJSService.InvokeFromStringAsync<int>("module.exports = (callback) => callback(undefined, NaN);");

Some would say returning 0 for 0, "NaN" and "undefined" is unintuitive as these values are not equivalent. Also, this approach means we return 0 if a script returns "randomstring", a float, an object or anything that can't be deserialized to int:

// Also returns 0
resultNum = await StaticNodeJSService.InvokeFromStringAsync<int>("module.exports = (callback) => callback(undefined, "anything");");

That would be unintuitive.

For now I prefer to throw if deserialization fails. This approach is easy to understand and predictable - if a value can't be deserialized to expected type, throw.

[irahopkinson]

Ok. I understand and can work with that. Updating the documentation to make that clearer and throwing better exceptions for unexpected values lets the developer know what to do.

[JeremyTCD]

Yeap agreed, it should be clearer. Leave this issue open for now, we'll close it when said improvements have been made.