Add GetJSModules support from #1858 #1911
Conversation
|
at this point I think that |
js/internal/modules/modules.go
Outdated
| @@ -61,3 +61,25 @@ func Register(name string, mod interface{}) { | |||
| } | |||
There was a problem hiding this comment.
The check above here is done only against the modules that are registered through the Register call.
While the current usage is such they are required to have x/ in the public modules module, it still isn't technically correct.
So should I:
- do nothing
- drop the internal
modulespackage and move everything to the public one. This way I will check it there and hope that nobody will addx/k6standard lib. - Actually check against the result of
GetJSModules
There was a problem hiding this comment.
Hhmm considering we only made the package internal to begin with was to distinguish between internal and external module registration, and internal modules don't call Register() anymore, I vote for the 2nd approach, and to do any additional checks in the public package.
There was a problem hiding this comment.
Not sure I grok everything fully, but yeah, 2nd approach seems best 👍, we don't need an internal module anymore.
There was a problem hiding this comment.
Mostly LGTM, besides the minor nitpicks mentioned inline and another more global one.
I think @simskij mentioned it somewhere, but we should probably move the HasModuleInstancePerVU interface out of the internal package, so that extensions can also explicitly check that they implement it. Not a deal breaker, we can do that a few versions from now and keep it in internal in v0.32.0, they can still implement it by just having the appropriate method. But if we don't intend to make any more changes, we probably can make it public even now.
| // GlobalHTTP is a global HTTP module for a k6 instance/test run | ||
| type GlobalHTTP struct{} | ||
|
|
||
| var _ modules.HasModuleInstancePerVU = new(GlobalHTTP) |
There was a problem hiding this comment.
nitpick: now that we're back to interface{}, this check like
var _ modules.HasModuleInstancePerVU = New()might be somewhat useful again
There was a problem hiding this comment.
well that will mean there will be an import loop ;)
There was a problem hiding this comment.
should I put in the modules package ?
This allows integration tests to have separate global module instance of all internal js modules. For this to be supported for extension an additional type/interface will be required a bit more added code
And drop the SharedObjects from the InitEnvironment
|
should I:
|
Codecov Report
@@ Coverage Diff @@
## master #1911 +/- ##
==========================================
+ Coverage 71.28% 71.42% +0.13%
==========================================
Files 183 183
Lines 14274 14245 -29
==========================================
- Hits 10175 10174 -1
+ Misses 3479 3439 -40
- Partials 620 632 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This is not needed anymore since #1911.
This is not needed anymore since #1911.
This allows integration tests to have separate global module instance of
all internal js modules.
For this to be supported for extension an additional type/interface will
be required a bit more added code