Modules fixes #2234

mstoykov · 2021-11-11T09:27:36Z

No description provided.

yorugac

This renaming looks much neater 👍

mstoykov · 2021-11-11T13:13:42Z

js/modules/modules.go

-type InstanceCore interface {
+// InstanceParams is something that will be provided to module instance on initialization so that it can get access to
+// everything it needs
+type InstanceParams interface {


Looking for a better name, after which I will rebase it to get rid fixup commits

Following the Go naming convention for interfaces, you would end up with something like ContextRuntimeGetter. Idiom aside, I find InstanceParams explicit enough, although I didn't expect/realize it was an interface until I checked how it was used: when seeing the InstanceParams argument in the NewModuleInstance it wasn't immediately clear it was an interface.

Hope that's helpful 👀

The name seems fine, and is more in line with output extensions. 👍

I'd just make the comment more descriptive. Maybe:

InstanceParams will be provided to module instances on initialization so that they can access internal k6 objects.

ModuleParams ?

@oleiade the problem is that it will not only be about Getting those 2 or even the actual 4 it''s giving now.

In #2228 this is also the way to add something to the event loop.

In practice it's interacting with k6, predominantly in the context of 1 VU and the current iteration. So something like State would've made sense except for the part that modules.State.AddToEventLoop sounds bad.

_tangent: AddToEventLoop might not be the end method that will be used btw, it might be what currently is named Reserve on the eventloop, which in practice returns a function taking a function (func(func()) where the idea is that it needs to be called to add something to the event loop but at the same time it will block the event loop from ending even if the loop is empty. (It doesn't actually reserve a position in the queue or something like that). Given that the name for that might be modules.State.blockEventLoopUntil or ... something actually better 🤣

@imiric
How about?

InstanceParams will be provided to module instances on initialization so that they can access interact with k6.

Again the eventLoop and the likely additional stuff for an module/VU lifecycle integration will be at least in part, part of this

@yorugac the full name currently is modules.InstanceParams and these are the params for when an instance of the module on each import "nameofmodule" is being evaluated. There currently are no params provided just when we load "nameofmodule" because it's part of the k6 build but isn't actually instanciated.

InstanceParams is suitable for the current design. But the thing is, this is not actually an instance's parameters. It's for providing a way for a module instance to access a VU's whereabouts.

I won't say we should call it VUWhereabouts or VUUniverse (you can type it with more Us) 😄 Maybe, it can be something like VUInternals 🤷

VUInternals seems like the best option so far

Yes, @mstoykov I meant this params weren't about instance per se... So I'd say 👍 for VUInternals too but: event loop is going to add methods for promises there -- so it won't be internals either
How large would that interface grow?

How large would that interface grow?

Given that it provides a way for the modules to interact with k6, as much as we need it to interact :). The alternative to this growing is to keep adding interfaces that either it implements or the modules.Instance implements so that we can call something on it 🤷

mstoykov · 2021-11-11T13:15:56Z

js/initcontext.go

-	if modV2, ok := mod.(modules.IsModuleV2); ok {
-		instance := modV2.NewModuleInstance(&moduleInstanceCoreImpl{ctxPtr: i.ctxPtr})
-		return i.runtime.ToValue(toESModuleExports(instance.GetExports())), nil
+	if modV2, ok := mod.(modules.Module); ok {


modV2 now sounds ... bad, but I either need to shadow mod or not name one of them mod/module :(

Just m? 😄 modV2 would be confusing...

imiric · 2021-11-11T13:45:38Z

LGTM, just minor suggestions. 👍 for making exports explicit.

inancgumus

Some suggestions.

inancgumus · 2021-11-11T13:15:45Z

js/modulestest/modulestest.go

-// GetContext returns internally set field to conform to modules.InstanceCore interface
-func (m *InstanceCore) GetContext() context.Context {
+// GetContext returns internally set field to conform to modules.InstanceParams interface
+func (m *InstanceParams) GetContext() context.Context {


Why not just Context()?

I would guess you want to change the interface (currently named modules.InstanceParams) not the test implementation?

inancgumus · 2021-11-11T13:16:49Z

js/modulestest/modulestest.go

-// GetInitEnv returns internally set field to conform to modules.InstanceCore interface
-func (m *InstanceCore) GetInitEnv() *common.InitEnvironment {
+// GetInitEnv returns internally set field to conform to modules.InstanceParams interface
+func (m *InstanceParams) GetInitEnv() *common.InitEnvironment {


Just Env or InitEnv?

inancgumus · 2021-11-11T13:16:57Z

js/modulestest/modulestest.go

 	return m.InitEnv
 }

-// GetState returns internally set field to conform to modules.InstanceCore interface
-func (m *InstanceCore) GetState() *lib.State {
+// GetState returns internally set field to conform to modules.InstanceParams interface


inancgumus · 2021-11-11T13:17:05Z

js/modulestest/modulestest.go

-// GetRuntime returns internally set field to conform to modules.InstanceCore interface
-func (m *InstanceCore) GetRuntime() *goja.Runtime {
+// GetRuntime returns internally set field to conform to modules.InstanceParams interface
+func (m *InstanceParams) GetRuntime() *goja.Runtime {


Runtime()?

inancgumus · 2021-11-11T14:16:42Z

js/modules/k6/metrics/metrics.go

@@ -37,7 +37,7 @@ import (

 type Metric struct {
 	metric *stats.Metric
-	core   modules.InstanceCore
+	core   modules.InstanceParams


Just an idea.

What about vu as a field name instead of core?

So it'll be natural to type:

state := m.vu.State()

Instead of:

state := m.core.GetState()

inancgumus · 2021-11-11T14:29:10Z

js/modules/k6/execution/execution.go

 func New() *RootModule {
 	return &RootModule{}
 }


What I'm going to say would probably be involved and may not be an easy task. And, I'm not deeply knowledgeable about the system and our goals, so this can be a fruitless suggestion, and it might not even work 🤷

Why don't we register in an extension without giving the k6 an instance of the module, like this:

func init() { k6modules.Register("k6/x/browser") }

With this, we won't have to stutter in tests, and we can more natural type something like this:

New(mii)

Instead Of:

New().NewModuleInstance(mii)

So the interface would look like this:

type IsModuleV2 interface { New(InstanceCore) Instance }

In an extension we can say:

type RootModule struct { modules.InstanceCore } var _ modules.IsModuleV2 = &RootModule{} func New(m modules.InstanceCore) *RootModule { return &RootModule{InstanceCore: m} }

Instead of:

type ( RootModule struct{} ModuleInstance struct { modules.InstanceCore } ) var ( _ modules.IsModuleV2 = &RootModule{} _ modules.Instance = &ModuleInstance{} ) func (*RootModule) NewModuleInstance(m modules.InstanceCore) modules.Instance { return &ModuleInstance{InstanceCore: m} } func New() *RootModule { return &RootModule{} }

Of course, with this, we need to create a struct (RootModule) of a module ourselves. But then again, we can standardize it like RootModule and instantiate it using reflection.

The biggest (and most obvious problem) is that this all si also used for extensions. And while for internal we can have it listed the way it currently works (and likely will still need to work) is that extensions register themselves in a init function and add the RootModule so that we can call the NewModuleInstance from it ... in reality we can just register the New function, but for historical reasons previous to this interface (which is not used by any non-internal extension) you just registered interface{} and then k6 did a bunch of reflect "magic" see #1802, which had a list of problems, the major one of which is ... nobody could understand how it works or why it doesn't work for something or ... why it was needed in some cases.

Hope this explains the problem enough, but I would recommend reading the provided issue.

As well as remove GenerateExports and make module authors to actually specify exactly what they will export.

na--

❤️ LGTM

inancgumus · 2021-11-12T08:34:42Z

js/initcontext.go

@@ -151,24 +151,25 @@ func (i *InitContext) Require(arg string) goja.Value {
 	}
 }

-type moduleInstanceCoreImpl struct {
+// TODO this likely should just be part of the initialized VU or at least to take stuff directly from it.
+type moduleVUImpl struct {


Can we just use moduleVU instead of moduleVUImpl?

I have some hopes I will be working on the TODO above this cycle, and it will just get completely removed

Rename modules.Instance#GetExports to Exports

ca70290

github-actions bot requested review from na-- and yorugac November 11, 2021 09:27

yorugac previously approved these changes Nov 11, 2021

View reviewed changes

mstoykov dismissed yorugac’s stale review via fc73d7c November 11, 2021 09:59

na-- previously approved these changes Nov 11, 2021

View reviewed changes

na-- added this to the v0.35.0 milestone Nov 11, 2021

mstoykov dismissed na--’s stale review via 1e4c50f November 11, 2021 13:08

mstoykov commented Nov 11, 2021

View reviewed changes

inancgumus reviewed Nov 11, 2021

View reviewed changes

mstoykov added 2 commits November 11, 2021 18:09

Rename modules.IsModuleV2 to modules.Module

840b905

Rename modules.InstanceCore to modules.VU and don't embed it

5efd660

As well as remove GenerateExports and make module authors to actually specify exactly what they will export.

mstoykov force-pushed the modulesFixes branch from 2cd7f24 to 5efd660 Compare November 11, 2021 16:17

mstoykov requested review from inancgumus, imiric, oleiade, yorugac, na-- and codebien November 12, 2021 08:23

na-- approved these changes Nov 12, 2021

View reviewed changes

inancgumus reviewed Nov 12, 2021

View reviewed changes

inancgumus approved these changes Nov 12, 2021

View reviewed changes

mstoykov merged commit 5b4fe2d into master Nov 12, 2021

mstoykov deleted the modulesFixes branch November 12, 2021 08:44

This was referenced Nov 15, 2021

Release k6 v0.35.0 #2227

Merged

Update k6 extension page grafana/k6-docs#490

Closed

mstoykov mentioned this pull request Nov 22, 2021

Discussion around ESM exports support #2258

Closed

na-- mentioned this pull request Feb 28, 2023

Deprecate old JS Module "API" #2949

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modules fixes #2234

Modules fixes #2234

mstoykov commented Nov 11, 2021

yorugac left a comment

mstoykov Nov 11, 2021

oleiade Nov 11, 2021

imiric Nov 11, 2021

yorugac Nov 11, 2021

mstoykov Nov 11, 2021

mstoykov Nov 11, 2021

inancgumus Nov 11, 2021 •

edited

Loading

mstoykov Nov 11, 2021

yorugac Nov 11, 2021

mstoykov Nov 11, 2021

mstoykov Nov 11, 2021

imiric Nov 11, 2021

imiric commented Nov 11, 2021

inancgumus left a comment

inancgumus Nov 11, 2021

mstoykov Nov 11, 2021

inancgumus Nov 11, 2021

inancgumus Nov 11, 2021

inancgumus Nov 11, 2021

inancgumus Nov 11, 2021

inancgumus Nov 11, 2021 •

edited

Loading

mstoykov Nov 11, 2021

na-- left a comment

inancgumus Nov 12, 2021

mstoykov Nov 12, 2021

Modules fixes #2234

Modules fixes #2234

Conversation

mstoykov commented Nov 11, 2021

yorugac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inancgumus Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imiric commented Nov 11, 2021

inancgumus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inancgumus Nov 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

na-- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inancgumus Nov 11, 2021 •

edited

Loading

inancgumus Nov 11, 2021 •

edited

Loading