promise: run callbacks on resolution

[robfig]

This would support ES Modules, since v8's interface returns a promise
when running a module to handle top-level await and imports.

The test does not currently pass:

--- FAIL: TestPromiseRejected (0.01s)
promise_test.go:81: expected Then to be called immediately on already-resolved promise, but was not

[robfig]

@rogchap @cleiner Hoping to get some feedback on the API and see if you had any tips on what I'm missing. I expected the callback to be invoked synchronously when registering it with Then, but that's not happening. I could check Promise.State() and invoke it myself, but that's a race condition.

[cleiner]

@robfig Then() with a single handler argument will not be called on rejected promises, you'd have to use Then(on_fulfilled, on_rejected) to make the function work as described. Also, if you look at the comment for the method in v8.h:4774, it says

If the promise is already resolved/rejected, the handler is invoked at the end of turn.

So I guess a call to PerformMicrotaskCheckpoint() is necessary after registering the callback.

Looking at the JavaScript Promise API, the current Go Then() is more like the finally() method. I think making the Go API mirror the JavaScript API would help match expectations in this case. So there'd be three functions: Catch, Then and Finally, each returning another Promise (not a Value).
Alternatively, to stick as close as possible to the C++ API, Finally could be omitted, so you'd have to call Catch and Then with the same handler or there could be a single Then(onFulfilled, onRejected) where either of the parameters may be nil.

[robfig]

Thanks for the guidance. PerformMicrotaskCheckpoint seems to be the crux of the issue.. I take it that v8 only makes progress on promises when that is called, rather than having some thread running in the background. How do you envision this fitting into the API? In other words, we return a Promise, and then whose responsibility is it to ask v8 to resolve the promise?

[robfig]

Referring to rusty_v8,
https://github.com/denoland/rusty_v8/blob/3c7ff01ad4a7ca14849be3ee2e8a0fc2b331bb15/src/isolate.rs#L45

I guess we would want to expose Explicit or Auto microtask execution on the Isolate too?

[codecov]

Codecov Report

Merging #118 (f5fb111) into master (4918644) will increase coverage by 0.16%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   96.63%   96.80%   +0.16%     
==========================================
  Files          12       12              
  Lines         416      438      +22     
==========================================
+ Hits          402      424      +22     
  Misses          9        9              
  Partials        5        5              

Impacted Files	Coverage Δ
context.go	94.36% <100.00%> (+0.24%)	⬆️
promise.go	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4918644...f5fb111. Read the comment docs.

[robfig]

Well, it looks like the default microtasks policy is Auto, so I'm not sure why it doesn't "just work"
https://github.com/v8/v8/blob/master/include/v8.h#L7416

Anyway, I added methods mirroring the C++ interface, and one call to process microtasks got all the tests passing. The only weirdness is that I have to add PerformMicrotasksCheckpoint to Context instead of Isolate because it requires the relevant context to register and deregister. I didn't see a way around that, but that didn't seem like such a big problem.

[robfig]

As an aside, I have no idea why some functions are marked as extern and others are not in v8go.cc/h

[cleiner]

Well, it looks like the default microtasks policy is Auto, so I'm not sure why it doesn't "just work"
https://github.com/v8/v8/blob/master/include/v8.h#L7416

From the description "microtasks are invoked when the script call depth decrements to zero" I'd assume that nothing will happen if it is already at zero when registering a promise callback. So even Auto requires some follow-on interaction with the engine (run script or process microtasks) in that case.

Anyway, I added methods mirroring the C++ interface, and one call to process microtasks got all the tests passing.

Since this behavior is probably non-obvious to v8go users, maybe add a note about the call to the comments of those methods?

As an aside, I have no idea why some functions are marked as extern and others are not in v8go.cc/h

Good question, as far as I know extern should not be needed as it is the default for functions anyway.

[robfig]

Makes sense, thanks. Added a note.

[rogchap]

Hey @robfig This is looking really good. Thank you.
If we can just address the panic and increase the code coverage then this is all good to merge.

[rogchap]

Would like to avoid panic within this library and also just ignore Callbacks if the length in > 2. Can we just return p and make it a noop?

[robfig]

OK. I think we really don't see eye to eye on this design question, since I feel adverse to this. IMO this goes against the typical design ethos & common practice.. that is, ignoring unexpected input and carrying on. But it's your library, and in this case the chance of mistakes by the caller seems low. Updated

[rogchap]

That's a fair comment and I agree. Would it be better to return an error? 🤔

The downside is you loose the ability to do method chaining (which is not generally idiomatic Go)

I'm just trying to avoid using panic

I'm open to other's input? @tmc @cleiner @zwang

[zwang]

I prefer not panic solution as this is a library to be used by other app. Good not to panic.

[robfig]

I think it would be better to panic, hence the code :)

I would refer you again to examples in the standard library:
https://pkg.go.dev/strings#Repeat

There are no fewer than 3 functions in the strings package that are documented to panic when given an invalid input. It would be frustrating for callers if they had to handle an error return unnecessarily. It's ok for the standard library to panic but not for v8go? The argument that panics are not allowed in public interfaces at all must not be telling the whole story.

Searching for more information on the topic, I agree with the point of view presented here by Axel:
https://groups.google.com/g/golang-nuts/c/5S324hgLdfI/m/XpWmVQGJCAAJ

Personally I've been using Go since 1.0 (... 10 years now?) and I've seen a lot of libraries. I do not believe the nil checks and error returns used in v8go are common practice at all. Can you point to popular packages that do this?

[rogchap]

Ok; I think we are all in agreement now. Sorry for the back-and-forth @robfig can you revert your last commit? ie. place back the panic.

[robfig]

Sure, happy to do it. Updated.

Last note - a lot of the errors returned in v8go are as a result of nil checks. In the standard library, it doesn't even perform nil checks, relying on the code to panic on nil as a side effect.

For example, it looks to me like io.WriteString(w Writer, s string) will panic if either of the arguments are nil.
https://golang.org/src/io/io.go?s=13817:13895#L307

I know there is a practice (which I've seen in Java in particular) to aggressively check arguments for nil on entry to every function. I haven't really seen that done in Go, and I can't say that I've missed it.

[mitar]

So it seem the conclusion is: panic is OK when arguments to the API are invalid, but for results of running JavaScript, there will still be err returned? So if JavaScript code throws a top-level exception, there will still be err and not panic?

[rogchap]

Yea, that seems like the right approach; JS exceptions would still return error

[mitar]

OK. So will you then change the API now? Like template.Set currently returns err. It seems it should not anymore and just panic?

[rogchap]

Hey @robfig are you able to address the test coverage?

[robfig]

Added a test case