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.

Sign up for GitHub

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

Contain main module within it's own scope as well #2571

Merged
merged 7 commits into from
Jul 18, 2022
Merged

Contain main module within it's own scope as well #2571

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e5941d5
Add to showcase how currently options is accessible everywhere
mstoykov Jun 13, 2022
35146ca
Contain main module within it's own scope as well
mstoykov Jun 14, 2022
42bf5d6
Drop TestBundleInstantiate/SetAndRun
mstoykov Jul 14, 2022
3e52cb8
Better panic message
mstoykov Jul 14, 2022
a614cfe
Extract main commonjs module initialization
mstoykov Jul 14, 2022
6b77bfc
Improve compiler readability around commonjs wrapping parameters
mstoykov Jul 14, 2022
7e289a5
Add a comment about main script map lookup
mstoykov Jul 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,7 @@ func TestVuInitException(t *testing.T) {

var exception errext.Exception
require.ErrorAs(t, err, &exception)
assert.Equal(t, "Error: oops in 2\n\tat file:///script.js:10:9(31)\n", err.Error())
assert.Equal(t, "Error: oops in 2\n\tat file:///script.js:10:9(30)\n\tat native\n", err.Error())

var errWithHint errext.HasHint
require.ErrorAs(t, err, &errWithHint)
Expand Down
55 changes: 39 additions & 16 deletions js/bundle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ type BundleInstance struct {
// TODO: maybe just have a reference to the Bundle? or save and pass rtOpts?
env map[string]string

exports map[string]goja.Callable

exports map[string]goja.Callable
moduleVUImpl *moduleVUImpl
pgm programWithSource
}

// NewBundle creates a new bundle from a source file and a filesystem.
Expand All @@ -90,7 +90,7 @@ func NewBundle(
Strict: true,
SourceMapLoader: generateSourceMapLoader(logger, filesystems),
}
pgm, _, err := c.Compile(code, src.URL.String(), true)
pgm, _, err := c.Compile(code, src.URL.String(), false)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -142,7 +142,7 @@ func NewBundleFromArchive(
CompatibilityMode: compatMode,
SourceMapLoader: generateSourceMapLoader(logger, arc.Filesystems),
}
pgm, _, err := c.Compile(string(arc.Data), arc.FilenameURL.String(), true)
pgm, _, err := c.Compile(string(arc.Data), arc.FilenameURL.String(), false)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -208,7 +208,8 @@ func (b *Bundle) makeArchive() *lib.Archive {

// getExports validates and extracts exported objects
func (b *Bundle) getExports(logger logrus.FieldLogger, rt *goja.Runtime, options bool) error {
exportsV := rt.Get("exports")
pgm := b.BaseInitContext.programs[b.Filename.String()]
Copy link
Member

Choose a reason for hiding this comment

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

Ugh, my other comment (#2571 (comment)) seems to be in a thread that's not visible in the diff anymore, so let me repeat it. Please add a comment and/or TODO here that explain why this is safe to do and how we can change it to not look so scary.

exportsV := pgm.module.Get("exports")
if goja.IsNull(exportsV) || goja.IsUndefined(exportsV) {
return errors.New("exports must be an object")
}
Expand Down Expand Up @@ -262,26 +263,31 @@ func (b *Bundle) Instantiate(logger logrus.FieldLogger, vuID uint64) (*BundleIns
}

rt := vuImpl.runtime
pgm := init.programs[b.Filename.String()]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest using a getter here instead of the map. For example, let's imagine there will be a case when nothing storing for the desired key, better in the case of stop execution, e.g., panic, whatever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in all the places where access this we either:

  1. know that there will be the key - otherwise we have some major bug
  2. we do check that it is there and have something else to do

Given that I am going to be rewriting even more of this in #2563 or following PRs and this will just increase the amount of abstraction here. Especially not now.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, don't increase the abstraction, but this naked map lookup is giving me anxiety 😅 So at least add a few words of comment and/or a TODO here, that we are sure this index exists because of so and so, etc.

bi := &BundleInstance{
Runtime: rt,
exports: make(map[string]goja.Callable),
env: b.RuntimeOptions.Env,
moduleVUImpl: vuImpl,
pgm: pgm,
}

// Grab any exported functions that could be executed. These were
// already pre-validated in cmd.validateScenarioConfig(), just get them here.
exports := rt.Get("exports").ToObject(rt)
exports := pgm.module.Get("exports").ToObject(rt)
for k := range b.exports {
fn, _ := goja.AssertFunction(exports.Get(k))
bi.exports[k] = fn
}

jsOptions := rt.Get("options")
jsOptions := exports.Get("options")
var jsOptionsObj *goja.Object
Copy link
Member

Choose a reason for hiding this comment

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

Hmm it would be relatively easy to make this in a DynamicObject that prints a warning if anyone tries to access any of the properties, right? 🤔 Telling people (with a sync.Once, to limit spam) to use test.options from k6/execution for Get(), Keys() and Has() operations, and telling (with another sync.Once) them that modifying the options during the script runtime is useless and won't actually affect anything?

Not in this PR, of course, but should I open an issue about that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's possible - not certain how well it will play once we have ESM, but it should work 🤷

Copy link
Contributor

@codebien codebien Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When I tried during the test.options implementation, there was an issue in the case of options exported as const, where IIRC it isn't compatible with dynamic solutions.

Copy link
Member

@na-- na-- Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

#2601 please comment/edit if I've missed something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm this in practice will not work ... all that well even with just this cchanges as we already can not change the original options. This here only sets exports.options in case they are empty.

even before that again we were just setting something globally.

This won't work with ESM as well as in that case just as now we don't have access to change waht options is in hte scope of the module.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really. In the case of commonjs transpiled by babel the actual code is something like

let options = {...}
exports.options = options;

// and then later it's accessed as `options` 
options.scenario.....

In this case we can resset exports.options but nothing will be using it. What we have done so far (and what this PR changes) is basically setting globalThis.options which just so happens to be the same as having let options in the global scope.

In ESM

export let options ={}

is more or less

let options = {}
export options;

And then we get access to options and can set it's fields (just as before) but AFAIK there is no way to just change what options is supposed to be.

You can think of it as modules are supposed to encapsulate stuff so that the outside world can't mangle them and what you want is basically what modules are kind of trying to prevent. (They also prevent spilling stuff from inside modules in the global scope which is the other side of the coin - arguably the one that is more commonly cited)

So it seems like this isn't really a thing we can do 🤷

Copy link
Member

Choose a reason for hiding this comment

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

Got it, I think... Then this whole logic of updating the script options with the consolidated values won't work at all? 😕

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe it will, since we're going item by item and updating individual keys of the object? 🤔 In which case, we might be able to add the usage warnings at the key level?

Copy link
Member

@na-- na-- Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I mean this:

k6/js/bundle.go

Lines 297 to 299 in 6b77bfc

if err := jsOptionsObj.Set(key, val); err != nil {
instErr = err
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should work for all varitants 🤔

if jsOptions == nil || goja.IsNull(jsOptions) || goja.IsUndefined(jsOptions) {
jsOptionsObj = rt.NewObject()
rt.Set("options", jsOptionsObj)
err := exports.Set("options", jsOptionsObj)
if err != nil {
return nil, fmt.Errorf("couldn't set exported options with merged values: %w", err)
}
} else {
jsOptionsObj = jsOptions.ToObject(rt)
}
Expand All @@ -302,12 +308,6 @@ func (b *Bundle) instantiate(logger logrus.FieldLogger, rt *goja.Runtime, init *
rt.SetFieldNameMapper(common.FieldNameMapper{})
rt.SetRandSource(common.NewRandSource())

exports := rt.NewObject()
rt.Set("exports", exports)
module := rt.NewObject()
_ = module.Set("exports", exports)
rt.Set("module", module)

env := make(map[string]string, len(b.RuntimeOptions.Env))
for key, value := range b.RuntimeOptions.Env {
env[key] = value
Expand All @@ -330,10 +330,27 @@ func (b *Bundle) instantiate(logger logrus.FieldLogger, rt *goja.Runtime, init *
init.moduleVUImpl.ctx = context.Background()
init.moduleVUImpl.initEnv = initenv
init.moduleVUImpl.eventLoop = eventloop.New(init.moduleVUImpl)
pgm := init.programs[b.Filename.String()] // this just initializes the program
pgm.pgm = b.Program
pgm.src = b.Source
exports := rt.NewObject()
pgm.module = rt.NewObject()
_ = pgm.module.Set("exports", exports)
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
init.programs[b.Filename.String()] = pgm
olegbespalov marked this conversation as resolved.
Show resolved Hide resolved

err = common.RunWithPanicCatching(logger, rt, func() error {
return init.moduleVUImpl.eventLoop.Start(func() error {
_, errRun := rt.RunProgram(b.Program)
return errRun
f, errRun := rt.RunProgram(b.Program)
if errRun != nil {
return errRun
}
if call, ok := goja.AssertFunction(f); ok {
if _, errRun = call(exports, pgm.module, exports); errRun != nil {
return errRun
}
return nil
}
panic("we shouldn't be able to get here")
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
})
})

Expand All @@ -344,6 +361,12 @@ func (b *Bundle) instantiate(logger logrus.FieldLogger, rt *goja.Runtime, init *
}
return err
}
exportsV := pgm.module.Get("exports")
if goja.IsNull(exportsV) {
return errors.New("exports must be an object")
}
pgm.exports = exportsV.ToObject(rt)
init.programs[b.Filename.String()] = pgm
unbindInit()
init.moduleVUImpl.ctx = nil
init.moduleVUImpl.initEnv = nil
Expand Down
13 changes: 7 additions & 6 deletions js/bundle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,11 +96,11 @@ func TestNewBundle(t *testing.T) {
_, err := getSimpleBundle(t, "/script.js", `throw new Error("aaaa");`)
exception := new(scriptException)
require.ErrorAs(t, err, &exception)
require.EqualError(t, err, "Error: aaaa\n\tat file:///script.js:1:7(2)\n")
require.EqualError(t, err, "Error: aaaa\n\tat file:///script.js:2:7(3)\n\tat native\n")
})
t.Run("InvalidExports", func(t *testing.T) {
t.Parallel()
_, err := getSimpleBundle(t, "/script.js", `exports = null`)
_, err := getSimpleBundle(t, "/script.js", `module.exports = null`)
require.EqualError(t, err, "exports must be an object")
})
t.Run("DefaultUndefined", func(t *testing.T) {
Expand Down Expand Up @@ -169,13 +169,13 @@ func TestNewBundle(t *testing.T) {
// ES2015 modules are not supported
{
"Modules", "base", `export default function() {};`,
"file:///script.js: Line 1:1 Unexpected reserved word",
"file:///script.js: Line 2:1 Unexpected reserved word (and 2 more errors)",
},
// BigInt is not supported
{
"BigInt", "base",
`module.exports.default = function() {}; BigInt(1231412444)`,
"ReferenceError: BigInt is not defined\n\tat file:///script.js:1:47(6)\n",
"ReferenceError: BigInt is not defined\n\tat file:///script.js:2:47(7)\n\tat native\n",
},
}

Expand Down Expand Up @@ -763,6 +763,7 @@ func TestBundleInstantiate(t *testing.T) {

t.Run("SetAndRun", func(t *testing.T) {
t.Parallel()
t.Skip("This makes no sense for a test we are basically testing that we can reset global")
mstoykov marked this conversation as resolved.
Show resolved Hide resolved
b, err := getSimpleBundle(t, "/script.js", `
export let options = {
vus: 5,
Expand Down Expand Up @@ -798,7 +799,7 @@ func TestBundleInstantiate(t *testing.T) {
bi, err := b.Instantiate(logger, 0)
require.NoError(t, err)
// Ensure `options` properties are correctly marshalled
jsOptions := bi.Runtime.Get("options").ToObject(bi.Runtime)
jsOptions := bi.pgm.exports.Get("options").ToObject(bi.Runtime)
vus := jsOptions.Get("vus").Export()
require.Equal(t, int64(5), vus)
tdt := jsOptions.Get("teardownTimeout").Export()
Expand All @@ -809,7 +810,7 @@ func TestBundleInstantiate(t *testing.T) {
b.Options.VUs = null.IntFrom(10)
bi2, err := b.Instantiate(logger, 0)
require.NoError(t, err)
jsOptions = bi2.Runtime.Get("options").ToObject(bi2.Runtime)
jsOptions = bi2.pgm.exports.Get("options").ToObject(bi2.Runtime)
vus = jsOptions.Get("vus").Export()
require.Equal(t, int64(10), vus)
b.Options.VUs = optOrig
Expand Down
10 changes: 2 additions & 8 deletions js/compiler/compiler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,7 @@ func (c *Compiler) Compile(src, filename string, main bool) (*goja.Program, stri
// additioanlly it fixes off by one error in commonjs dependencies due to having to wrap them in a function.
func (c *compilationState) sourceMapLoader(path string) ([]byte, error) {
if path == sourceMapURLFromBabel {
if !c.main {
return c.increaseMappingsByOne(c.srcMap)
}
return c.srcMap, nil
return c.increaseMappingsByOne(c.srcMap)
}
c.srcMap, c.srcMapError = c.compiler.Options.SourceMapLoader(path)
if c.srcMapError != nil {
Expand All @@ -214,10 +211,7 @@ func (c *compilationState) sourceMapLoader(path string) ([]byte, error) {
c.srcMap = nil
return nil, c.srcMapError
}
if !c.main {
return c.increaseMappingsByOne(c.srcMap)
}
return c.srcMap, nil
return c.increaseMappingsByOne(c.srcMap)
}

func (c *Compiler) compileImpl(
Expand Down
7 changes: 4 additions & 3 deletions js/initcontext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,10 @@ import (
)

type programWithSource struct {
pgm *goja.Program
src string
module *goja.Object
pgm *goja.Program
src string
module *goja.Object
exports *goja.Object
}

const openCantBeUsedOutsideInitContextMsg = `The "open()" function is only available in the init stage ` +
Expand Down
12 changes: 6 additions & 6 deletions js/initcontext_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,13 @@ func TestInitContextRequire(t *testing.T) {
bi, err := b.Instantiate(logger, 0)
assert.NoError(t, err, "instance error")

exports := bi.Runtime.Get("exports").ToObject(bi.Runtime)
exports := bi.pgm.exports
require.NotNil(t, exports)
_, defaultOk := goja.AssertFunction(exports.Get("default"))
assert.True(t, defaultOk, "default export is not a function")
assert.Equal(t, "abc123", exports.Get("dummy").String())

k6 := bi.Runtime.Get("_k6").ToObject(bi.Runtime)
k6 := exports.Get("_k6").ToObject(bi.Runtime)
require.NotNil(t, k6)
_, groupOk := goja.AssertFunction(k6.Get("group"))
assert.True(t, groupOk, "k6.group is not a function")
Expand All @@ -96,7 +96,7 @@ func TestInitContextRequire(t *testing.T) {
bi, err := b.Instantiate(logger, 0)
require.NoError(t, err)

exports := bi.Runtime.Get("exports").ToObject(bi.Runtime)
exports := bi.pgm.exports
require.NotNil(t, exports)
_, defaultOk := goja.AssertFunction(exports.Get("default"))
assert.True(t, defaultOk, "default export is not a function")
Expand Down Expand Up @@ -130,7 +130,7 @@ func TestInitContextRequire(t *testing.T) {
require.NoError(t, afero.WriteFile(fs, "/file.js", []byte(`throw new Error("aaaa")`), 0o755))
_, err := getSimpleBundle(t, "/script.js", `import "/file.js"; export default function() {}`, fs)
assert.EqualError(t, err,
"Error: aaaa\n\tat file:///file.js:2:7(3)\n\tat go.k6.io/k6/js.(*InitContext).Require-fm (native)\n\tat file:///script.js:1:0(14)\n")
"Error: aaaa\n\tat file:///file.js:2:7(3)\n\tat go.k6.io/k6/js.(*InitContext).Require-fm (native)\n\tat file:///script.js:1:0(15)\n\tat native\n")
})

imports := map[string]struct {
Expand Down Expand Up @@ -282,7 +282,7 @@ func TestInitContextOpen(t *testing.T) {
t.Parallel()
bi, err := createAndReadFile(t, tc.file, tc.content, tc.length, "")
require.NoError(t, err)
assert.Equal(t, string(tc.content), bi.Runtime.Get("data").Export())
assert.Equal(t, string(tc.content), bi.pgm.exports.Get("data").Export())
})
}

Expand All @@ -291,7 +291,7 @@ func TestInitContextOpen(t *testing.T) {
bi, err := createAndReadFile(t, "/path/to/file.bin", []byte("hi!\x0f\xff\x01"), 6, "b")
require.NoError(t, err)
buf := bi.Runtime.NewArrayBuffer([]byte{104, 105, 33, 15, 255, 1})
assert.Equal(t, buf, bi.Runtime.Get("data").Export())
assert.Equal(t, buf, bi.pgm.exports.Get("data").Export())
})

testdata := map[string]string{
Expand Down
Loading