Javascript.NodeJS
Javascript.NodeJS copied to clipboard
Callback arguments are not always optional
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.
Hey you're right, there are some issues here.
-
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", anInvocationException
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.
-
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();");
-
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.
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.
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 string
s and stream
s, it serializes to JSON on the Node.js side and deserializes from JSON on the C# side. This goes for int
s, double
s, object
s 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.
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.
Yeap agreed, it should be clearer. Leave this issue open for now, we'll close it when said improvements have been made.