Separate Sobek and k6 logic from the module logic

[inancgumus]

This can also mitigate some of the known bugs caused by the concurrent Sobek usage throughout the extension 🤞

Runtime is most needed because of Sobek conversions. If we can provide a translation layer between Sobek and the extension code, we can greatly simplify it. For example:

func (b *BrowserContext) WaitForEvent(event string, optsOrPredicate sobek.Value) interface{} {
    rt := k6common.GetRuntime(b.ctx)

    if optsOrPredicate != nil && !sobek.IsUndefined(optsOrPredicate) && !sobek.IsNull(optsOrPredicate) {
        switch optsOrPredicate.ExportType() {
        case reflect.TypeOf(sobek.Object{}):
            opts := optsOrPredicate.ToObject(rt)
            for _, k := range opts.Keys() {
                switch k {
                case "predicate":
                    predicateFn, isCallable = sobek.AssertFunction(opts.Get(k))
                    if !isCallable {
                        k6Throw(b.ctx, "expected callable predicate")
                    }
                case "timeout":
                    timeout = time.Duration(opts.Get(k).ToInteger()) * time.Millisecond
                }
            }

This kind of cluttering happens because of internally exposing Sobek, whereas we could do:

// Extension mechanics package (actual logic here)

package browser

func (c *Context) WaitForEvent(event string, predicate func()) (Event, error) {
    ...
    return predicate(c.page)
}

// Translation layer (Sobek -> extension) — It's now named as the mapping layer.

type BrowserContext struct {
    ...
    bc browser.Context
    ...
}

func (b *BrowserContext) WaitForEvent(event string, optsOrPredicate sobek.Value) interface{} {
    // sobek stuff here
    r, err := b.bc.WaitForEvent(event predicate)
    if err != nil {
        k6Throw(...)
    }   
}

---

                      Test script
                           │
                           │
┌──────────────────────────▼────────────────────────────┐
│            Exposed methods to the test script         │
│          Extension translation layer: JS -> Sobek     │
└──────────────────────────┬────────────────────────────┘
                           │
┌──────────────────────────┴────────────────────────────┐
│                          ▼                            │
│                                                       │
│               Extension logic (Sobek-free)            │
│                                                       │
│                          │                            │
└──────────────────────────┼────────────────────────────┘
                           │
                           │
             ┌─────────────┼─────────────┐
             │             ▼             │
             │                           │
             │          Browser          │
             │                           │
             │                           │
             └───────────────────────────┘

See https://github.com/grafana/xk6-browser/pull/268#discussion_r822566026.

[imiric]

This is also important to avoid data races from sharing goja.Runtime. See https://github.com/grafana/xk6-browser/issues/254#issuecomment-1060380139 and https://github.com/grafana/xk6-browser/pull/263#pullrequestreview-905621976. The quickest relief would be to remove it from ElementHandle.eval().

[inancgumus]

quickest relief would be to remove it from ElementHandle.eval()

What's on your mind doing so? :)

[imiric]

Well, we currently pass goja.Values to this function, and to ExecutionContext.eval(), and do the conversion inside it:

https://github.com/grafana/xk6-browser/blob/3ad187ce3f3cd07fd572bb504f8a69d28cf29d4f/common/execution_context.go#L180

https://github.com/grafana/xk6-browser/blob/3ad187ce3f3cd07fd572bb504f8a69d28cf29d4f/common/helpers.go#L55

Instead, this should happen much higher up in the stack, as close as possible or inside the methods we already expose to JS, via this JS->Go translation layer. This way we should(?) ensure a single goroutine has access to the goja.Runtime, and then when used internally we can worry about our own data races.

[inancgumus]

+1 That is exactly what I had in my mind: Goja should not enter into the extension logic realm.