Open aminmarashi opened 4 years ago
It's unclear what the best way to proceed is. Async implementations that accept variable numbers of arguments should previously have been using Array.prototype.pop.call(arguments)
to obtain callback
; they are currently broken as @aminmarashi-binary reports, but:
callback
via a named argument.stepCallExpression
append any trailing arguments to argsWithCallback
would make both named-arguments and varargs implementations work, with the proviso that it would be impossible for the implementation to detect if any named argument was omitted, and creating the horrible situation of having the callback be infixed amongst the user-supplied arguments.stepCallExpression
put the callback first breaks all existing code but corrects a serious design flaw. The breakage should be difficult to abuse, since although the callback could end up (incorrectly) being saved in some user-accessible location, no user-supplied argument will be callable, so async function implementations will definitely crash when they call callback
if their named parameters haven't been updated, preventing any further user code from being executed. This might be preferable to further hackery, since I suspect that many existing usages are probably broken in more subtle ways at the present time.I should note that although async function callbacks work much like Promise
s, the latter are for various reasons not quite the right fit in this situation; I created PR #178 to provide a similar but more appropriate mechanism to allow async functions to throw.
Hey, can i work on this issue.
@pakya-1909: Thanks for the offer. I think this issue is waiting for @NeilFraser to make a decision about whether he is willing to break backwards compatibility to fix varargs or not.
How about adding an optional falsely attribute to createAsyncFunction, making it createAsyncFunction (asyncFunc, varArgs)
, setting func.varArgs = varArgs
.
Then in stepCallExpression:
if (func.varArgs) {
// call with callback first
} else {
// call as before
}
Adds a little overhead for an additional if, but should be backwards compatible with existing code. (Would only break existing code if they were erroneously passing more than one argument to createAsyncFunction.)
How about adding an optional falsely attribute to createAsyncFunction, making it
createAsyncFunction (asyncFunc, varArgs)
, settingfunc.varArgs = varArgs
.
An excellent suggestion.
I do wonder if it might be preferable to subsume any new async function support machinery in the work you're doing to support calling interpreted code from native functions, though: see google/CodeCity#172 for what I have in mind.
It might be useful to get @NeilFraser to weigh in on this, when he has a moment.
though: see google/CodeCity#172 for what I have in mind.
Sorry, I'm a little confused on what I should be looking at in that link.
Sorry, I'm a little confused on what I should be looking at in that link.
Mainly, as an example, the addition of FunctionResult.Block
to allow any NativeFunction
(not just AsyncFunction
s) to block while awaiting a (natively async) callback. There is a good example of how this is used in commit 8f7c760.
It basically makes AsyncFunctions obsolete. It means that we wouldn't need to make any changes to the existing createAsyncFunction
API to add new features like varargs support, because all such API changes could be confined to a new and improved NativeFunction API.
Consider the following code example:
If executed, the above code throws this error:
The reason seems to be this commit which forces the position of callback to be always the same as the function definition.
I can see the point of the above commit, but would be nice if we could somehow find a workaround for this issue as well.
I wrote a workaround for this issue, which is enough for my existing use-case:
The biggest issue with the above workaround is that it makes every function args size = 101, that is not a very nice and efficient solution.
Instead of the above solution, I would like to propose adding support for async functions that return
Promise
to the interpreter code, another advantage to this is that thePromise
abstraction can also be used to distinguish failure and successful results.If we use
Promise
, we don't need to pass the callback and therefore we can leave the arguments unchanged.I think because the
Promise
object is created outside of the interpreter code, this means the existing code will still work on older browsers.I would love to create a PR with whatever solution you think is best.