Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common.bind replacement #2108

Merged
merged 14 commits into from
Aug 25, 2021
Merged

common.bind replacement #2108

merged 14 commits into from
Aug 25, 2021

Conversation

mstoykov
Copy link
Contributor

@mstoykov mstoykov commented Aug 5, 2021

mstoykov and others added 10 commits August 5, 2021 16:09
This goes all the way and tries (unfortunately not very well, I will try
again) to make the user embed the ModuleInstance it gets in the return
module so that it always has access to the Context and w/e else we
decide to add to it.

I also decided to force some esm along it (this is not required) to
test out some ideas.
Co-authored-by: Ivan Mirić <ivan@loadimpact.com>
@mstoykov
Copy link
Contributor Author

mstoykov commented Aug 5, 2021

This is #2101 but not based on the metrics registry changes as it seems we won't merge them fast enough but #2093 is important and should land with the full k6/execution for this release.

Also I will likely squash everything when merging this once it's approved

Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, no major comments. 👍

How the module struct needs to be defined now is a bit convoluted, and GenerateExports() is somewhat hairy, but nowhere near the mess of common.Bind() 😅

js/modules/k6/metrics/metrics.go Outdated Show resolved Hide resolved
js/modules/k6/metrics/metrics_test.go Outdated Show resolved Hide resolved
mstoykov and others added 2 commits August 5, 2021 18:26
Co-authored-by: Ivan Mirić <ivan@loadimpact.com>
imiric
imiric previously approved these changes Aug 6, 2021
js/modules/modules.go Outdated Show resolved Hide resolved
imiric pushed a commit to grafana/xk6-execution that referenced this pull request Aug 6, 2021
imiric pushed a commit to grafana/xk6-execution that referenced this pull request Aug 6, 2021
@@ -69,31 +65,108 @@ type HasModuleInstancePerVU interface {
NewModuleInstancePerVU() interface{}
}

// IsModuleV2 ... TODO better name
type IsModuleV2 interface {
NewModuleInstance(InstanceCore) Instance
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make NewModuleInstancePerVU() obsolete? Essentially every V2 module will return a new instance per VU?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, but we still use it, and somebody else, might too, so I prefer to deprecate it at a later point and remove at an even later one

js/initcontext.go Outdated Show resolved Hide resolved
Comment on lines +125 to +130
// Exports is representation of ESM exports of a module
type Exports struct {
// Default is what will be the `default` export of a module
Default interface{}
// Named is the named exports of a module
Named map[string]interface{}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mstoykov I'm having an issue with exports for k6/x/execution, see here.

With --compatibility-mode=extended and ES6's import everything works as expected. But with --compatibility-mode=base and ES5's require(), default is not exported... by default, and I need to manually reference it. I'm not defining Named since the module doesn't export any methods and everything should work via properties of the proxy.

I'm not sure if this is acceptable when working with ES5, or if the way we handle exports here should take that into account. Maybe Named should also be interface{} and we'd need more of common.Bind's reflection to make it work?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay some explanation on commonjs vs ESM:

In the case of commonjs, modules.exports (or just exports) is what is returned when you do require("modulename") and ... that is that. The major magic is that if you set something to modules.exports, the alias exports will magically be set to the same value as it's somekind of an alias. Other than that require acts exactly as a function would. And this was somewhat what we just did before that ... we just returned one big object which was just returned back to the script.

ESM on the other hand is ... well not at all the same. Not only is there no object to set values to, it's all special syntax and it has this concept of default and not default(named) exports, which also have a separate way of importing.

It just so happens that babel will try to guess based on the syntax and what it gets from require whether it has to use default or named exports somewhere and also when it sees export something it will set modules.exports accordingly including with the __esModule field. So the code in this PR just emulates what babel does when it exports on its own as much as possible and allows there to be a difference between what is default and not default export.

I am not really certain what a good solution will look like especially as I have mentioned numerous times that compatibility-mode=base is now less and less useful and hopefully one day when we have import/export syntax natively we can make it even less relevant.

I did have some worries about the usage of compatibility-mode=base myself, but as I used to set Default and Named to the same thing for k6/metrics it works the same as before.

I guess the easiest solution is to if Named is nil to just put default as the whole return and not set __esModule. (and maybe the reverse if Default is nil 🤷 ). This will later need to change significantly again once we know what is require and what is import ... from ....

I did wonder briefly if I should make some Proxy/DynamicObject which if it gets __esModule accessed change its form so it can act as both ... but as it wasn't needed I decided against it ... but I guess it will work 🤷 .

I think the Named check is probably going to be all we need though 🤔 , can you try it @imiric ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. We should mention some of that in the comment next to __esModule 😉

You're right, that seems like the simplest fix for now, see da1fb43. Works fine for k6/x/execution 👍

Co-authored-by: Mihail Stoykov <M.Stoikov@gmail.com>

See #2108 (comment)
imiric pushed a commit to grafana/xk6-execution that referenced this pull request Aug 6, 2021
imiric pushed a commit to grafana/xk6-execution that referenced this pull request Aug 6, 2021
na--
na-- previously approved these changes Aug 12, 2021
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM in general, though I left a minor inline nitpick and 👍-ed some of @imiric's comments

@mstoykov
Copy link
Contributor Author

@imiric @na-- does caae962 fixes all the comments you wanted ?

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, or at least I can't think of a way to make this better...

@mstoykov mstoykov merged commit 34a7743 into master Aug 25, 2021
@mstoykov mstoykov deleted the feat/common.BindReplacement branch August 25, 2021 14:47
@na-- na-- added this to the v0.34.0 milestone Sep 7, 2021
imiric pushed a commit to grafana/xk6-browser that referenced this pull request Nov 8, 2021
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.
inancgumus pushed a commit to grafana/xk6-browser that referenced this pull request Nov 9, 2021
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.
inancgumus pushed a commit to grafana/xk6-browser that referenced this pull request Nov 9, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants