js/encoding: ModuleV2 migration

[codebien]

Migrated k6/encding to the modules.Module (aka modules.ModuleV2) interface.

[CLAassistant]

All committers have signed the CLA.

[mstoykov]

LGTM, I still (as in the other PRs) am not certain if we:

• shouldn't stop exporting stuff that no longer need to be exported
• just return map[string]interface{} instead of creating a goja.Object that is basically that.

[yorugac]

Not sure about exports but regarding this:

• just return map[string]interface{} instead of creating a goja.Object that is basically that.

I think it's a matter of readability too: goja.Object is a "clear" entity on its own, but map[string]interface{} could be anything.

[oleiade]

I agree with @yorugac here. I also think explicit is better than implicit on the front of types.

[oleiade]

LGTM 👍

[codebien]

I think it's a matter of readability too: goja.Object is a "clear" entity on its own, but map[string]interface{} could be anything.

This is true for the specific line, but I think that the map exported directly from the Exports method adds some important benefits, check the latest fixup:

• We have removed the obj field from the instance, so the instance is the object and not a wrapper of another object. It should be clearer in terms of architecture.
• Export method from Exports should be better in terms of readability instead of Export them from the NewInstanceModule.

Not visible from the fixup but from other PRs:

• It's not required to add the mustExport method

[mstoykov]

I think it's a matter of readability too: goja.Object is a "clear" entity on its own, but map[string]interface{} could be anything.

goja.Object can be anything ;) map[string]interface{} can only be a map ;).

So the argument for me goes the other direction in two different ways:

1. goja.Object can truly be anything you can make it act as w/e you want which lets you w/e you want which also means that whoever reads this code now needs to actually look at whether you do something funky - which is what i went to look for, like I expected that it was goja.Object so that someone can't change an import, but that wasn't it we just made a goja.Object and then used it for the same thing that map is used for
2. the way the map[string]interface{} can be written directly in the Export makes it IMO a lot easier to understand what it's doing and in case like this where there is nothing special it's the better option IMO

Around #2258 we might decided to make this all more ... robust and with more specific types, maybe having something like nodejs' module.syncBuiltinESMExports(), but given that this PR just moves from the old common.Bind to the new modules.Module based approach and in the old one this is map[string]interface{} if anything it's probably better to not more changes than necessary

[yorugac]

For the sake of this discussion I just wanted to note the possibility of future renamings of those types but it seems @mstoykov already did that 🙂 I.e. there're pros and cons as mentioned and I also think this question should be revisited separately, like in #2258 as it simply has larger scope than this PR.