Map k6 browser API to Goja #661
Comments
|
We use xk6-browser/common/field_name_mapper.go Lines 14 to 17 in d450671 Instead of using |
I guess you can also try to combine them and have an object that maps |
|
@mstoykov it does indeed looks like we're only extending from the base that is implemented in func NewFieldNameMapper() *FieldNameMapper {
return &FieldNameMapper{
parent: k6common.FieldNameMapper{},
}
}Without this we will not be able to perform the following in the js test scripts: import { chromium } from 'k6/x/browser';
export default function () {
const browser = chromium.launch({headless: false});
const context = browser.newContext();
const page = context.newPage();
page.goto('https://test.k6.io', { waitUntil: 'networkidle' })
.then(() => {
const a = page.$('tr'); // <- This here
console.log(a.textContent());
const b = page.$$('tr'); // <- This here
console.log(b[1].textContent());
})
.finally(() => {
page.close();
browser.close();
});
}INFO[0002]
GET
/contacts.php
Short public page.
source=console
INFO[0002]
GET
/news.php
Longer public page (slower)
source=consoleWhich would break the existing API as well as the compatibility with playwright. Happy to discuss any other suggestions. |
Yes and by doing that changing how goja maps go struct fields and methods for everybody. Arguably the current "base" has way too many "corner cases" as is and you make it so that if anybody has Given that this arguably is a breaking change I am highly against you doing that.
That is only if you continue to just return a plain go object instead of wrapping it in a Goja one as we do in ... a bunch of places. It definitely is a lot easier (as you don't do anything) to just return the struct and have goja map the fields and methods automathically but I would argue that in cases like this one we shouldn't actually enforce everybody to have their methods renamed. |
|
@mstoykov Thanks for the detailed info, and the linked examples are helpful. Do you suggest not doing anything about this issue right now? 🤔 |
|
I am suggesting
At this point this looks like you will need to make a goja object and set it to have properties And then also "map" all the other methods as well. And then return that wherever those types are returned to goja. From my memories and courtesy look right now it seems like that will be a lot of code changes as you are currently depending really heavily on the fact that the automatic renaming works and there is more or less no "boundary" between goja facing code and go facing one. But given that mappign |
|
I'm taking a look at this now. I think we should attempt it at least. 🤞 |
inancgumus
changed the title
rt.SetFieldNameMapper
inancgumus
added
the
next
|
I believe we shouldn't change the current code and tests along with them. Instead, we can put a layer on top of the existing code that returns Goja with query mappings ( |
|
A couple of POC i ran yesterday: This was placed in rt := b.vu.Runtime()
o := rt.NewObject()
must := func(err error) {
if err != nil {
common.Throw(rt, err)
}
}
must(o.DefineDataProperty("goto", rt.ToValue(p.Goto), goja.FLAG_FALSE, goja.FLAG_FALSE, goja.FLAG_TRUE))
must(o.DefineDataProperty("$$", rt.ToValue(p.QueryAll), goja.FLAG_FALSE, goja.FLAG_FALSE, goja.FLAG_TRUE))
must(o.DefineDataProperty("close", rt.ToValue(p.Close), goja.FLAG_FALSE, goja.FLAG_FALSE, goja.FLAG_TRUE))
return oAlso this, which checks the type of the object before allowing the mapping from func (fm *FieldNameMapper) MethodName(t reflect.Type, m reflect.Method) string {
if exception, ok := methodNameExceptions[m.Name]; ok {
if strings.Contains(t.String(), "common.ElementHandle") ||
strings.Contains(t.String(), "common.Page") ||
strings.Contains(t.String(), "common.Frame") {
return exception
}
}
return fm.parent.MethodName(t, m)
}@mstoykov what about a stop gap where we check the type before allowing the mapping? In the next cycle we can work on the long term solution, which @inancgumus is suggesting. Also, is there a way of converting a go object to a |
|
Maybe with something like this? We take the existing object, clone it, and augment it with new methods without touching the original object. This has the benefit of wrapping the current extension instead of buggy-wise changing things everywhere. This is just a dirty POC. We need to make it clean 😅 import { chromium } from 'k6/x/browser';
export const options = {}
export default function() {
// both ways work:
const browserx = chromium.launch();
const browser = chromium.$();
browserx.close();
browser.close();
}func (*RootModule) NewModuleInstance(vu k6modules.VU) k6modules.Instance {
return &ModuleInstance{
mod: &JSModule{
Chromium: newBrowserType(vu),
Devices: common.GetDevices(),
Version: version,
},
}
}
func newBrowserType(vu k6modules.VU) *goja.Object {
// provide a wrapper instead of changing the existing code
bw := &browserWrapper{
BrowserType: chromium.NewBrowserType(vu),
}
// clone the existing browser type value
rt := vu.Runtime()
nobj := rt.NewObject()
obj := rt.ToValue(bw.BrowserType).ToObject(rt)
for _, k := range obj.Keys() {
nobj.Set(k, obj.Get(k))
}
// augment it with additional methods/properties
nobj.Set("$", rt.ToValue(bw.BrowserType.Launch))
return nobj
}
type browserWrapper struct {
api.BrowserType
}@mstoykov can probably shed more light on this. |
|
Both of those are possible: I am against stop gap solution given that as far as I am concerned we are not in a hurry. I would prefer this to be solved in some ... more future proof way. If we are going to go with a stop gap solution - dropping Given that I would recommend just doing the simple thing and creating an object with the provided fields one by one. This also will be way easier to find if anybody wonders how this is implemented as it will be right there in the codebase instead of some magical thing goja does that we then hack around/with. I would even kind of recommend to maybe look into creating a full blown JS types (with Looking at https://github.com/microsoft/playwright/blob/main/packages/playwright-core/src/client/frame.ts I do wonder if it was ever considered if we can use some of the code there. In other words to implement the things the OG playwrigth codebase needs and use (parts of) it to actually implement the playwright API 🤔 ? This likely is too much work for this particular case, but still might be something that should be thought about. |
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
Please take a look at #673 where I manually map the fields.
However, not sure the current solution will be future-proof. Since we have a relatively large API surface area; it can be error-prone to manually do this kind of mappings. I believe we might want to automate this in the future (maybe by writing a source-code generator). That way the resulting mapping can be in the code itself and people can easily track them without magic. |
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
As discussed in issue #661, we're now mapping the browser module methods to Goja. The global Goja field name mappers are removed from the code. The current mappers are just forwarders to the inner methods, and we add $ and $$ for Query and QueryAll methods. Let me know if I missed something while mapping the methods.
|
Reverted. We will re-revert this on January, 2th, 2023. And then start adding more JS tests. |
xk6-browser should not call
rt.SetFieldNameMapper.SetFieldNameMapper changes how goja maps properties from go types to js objects.
Given that this is global for the whole goja runtime, doing this in an extension is not a good idea and k6 already does it Internally.
This likely will need changes to xk6-browser as I have not looked into what exactly xk6-browser currently differs from k6
The text was updated successfully, but these errors were encountered: