Update extension to ModuleV2, minor context changes
#57
Conversation
| } | ||
| } | ||
|
|
||
| func (m *JSModule) Launch(ctx context.Context, browserName string, opts goja.Value) api.Browser { | ||
| // GetExports returns the exports of the JS module. | ||
| func (mi *ModuleInstance) GetExports() k6modules.Exports { |
There was a problem hiding this comment.
I prefer this to be just Exports() instead of Java-like GetExports. But it's been defined in the interface, so 🤷
Why does the JS module need to export? Is it for using the exported modules from JS scripts? Can we extend this comment and explain the why?
There was a problem hiding this comment.
Yeah, Exports() would be better, but difficult to change now as all extensions would need to be changed as well. The ModuleV2 name is also temporary and we plan to clean this up eventually, so we could consider changing to Exports() then as well.
Why does the JS module need to export?
So that it's actually usable from JS :)
Can we extend this comment and explain the why?
Like I mentioned in the other comment, this is not the place to give the background on the ModuleV2 structure or interfaces. We could expand the documentation in k6 itself (js/modules/modules.go), but the "why" is really answered in that commit and PR I linked to.
There was a problem hiding this comment.
I believe it's better to give some (a little) background/reason for an extension developer who is supposed to focus on this extension code only.
Maybe something like:
// GetExports returns the exports of the JS module.
// So that the test scripts can access the exported k6 modules.
// (^-- idk whether this comment is right or wrong. you can provide something better)
func (mi *ModuleInstance) GetExports() k6modules.Exports { ... }There was a problem hiding this comment.
I expanded the comment slightly in 1976e55.
This is the context passed from k6, and for synchronization purposes it would be better if the extension depended on it, so that errors like the `panic: send on close channel` from issue 49 wouldn't happen. This technically probably doesn't fix that yet, since the context created in k6 isn't synchronized with the Engine shutdown and closing of the SampleContainer channel, but it seems like a good idea regardless.
This is done to move the extension to the new way of structuring JS modules, and standardize how the main context and goja runtime are accessed. For background take a look at grafana/k6@34a7743fb, grafana/k6#2108, and the "Advanced JavaScript extension" section in https://k6.io/docs/misc/k6-extensions/#writing-a-new-extension. The tldr; is that this structure allows JS extensions to access internal k6 objects, is a saner alternative to `common.Bind()` (which we plan to deprecate eventually), and has the wanted side effect that each VU gets a separate instance of the module.
imiric
force-pushed
the
feat/js-modulev2
branch
from
796d777 to
1976e55
Compare
Update extension to `ModuleV2`, minor context changes
This updates the JS module to use the new
ModuleV2structure, and reuses the context passed tolaunch().It's an initial step towards fixing the
panic: send on closed channelmentioned in #49, but it will need some changes on the k6 side to synchronize the k6Engineshutdown with extensions that usestats.PushIfNotDone(). Likely the proper context will need to be propagated to extensions which doesn't seem to be the case right now.For background on the
ModuleV2changes take a look at grafana/k6@34a7743fb, grafana/k6#2108, and the "Advanced JavaScript extension" section in https://k6.io/docs/misc/k6-extensions/#writing-a-new-extension. The tldr; is that this is a new way of structuring JS modules that allows them to access internal k6 objects, is a saner alternative tocommon.Bind()(which we plan to deprecate eventually), and has the wanted side effect that each VU gets a separate instance of the module.