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

mstoykov · 2022-06-14T13:30:25Z

This is the continuation of #2566 with a fix in which I wrap the main module the same way every other module.

This more or less gets us inline with what will be the behaviour once ESM lands from #2563.

As you can see I had to also change other tests as they make no sense once the main module doesn't just work in the global scope.

This still needs work if we decide to merge it:

I will try to fix the lines for main modules(and not only) with no source maps and/or not going through babel.
More polishing - I likely will want to refactor some of the code a bit more. Like getExports seems like of strangely out of place which I also noticed while working on ESM

codebien · 2022-06-15T09:43:57Z

I agree we should act on this before we introduce the ESM. I'm not sure if applying a so important breaking change without any deprecation period is fine in terms of UX.
Do we have cons in doing it (except if someone is using this directly as a feature)?

Evaluating possible alternatives, do we have the option to keep the same current behaviour but with the addition of a print warning? So we could have this logic deprecated now and removed when the ESM will be introduced.

mstoykov · 2022-06-30T12:43:03Z

Given that this is clearly not what should've been happening - having access to another modules/script variables without importing it I think:

there likely will be no one actually using it on purpose
all the people who use it by accident likely will be better off finding out this was buggy and likely not what they intended.
the major breaking change IMO is that options will now not be available if you haven't export let options = {} which to be honest was previously very strange behaviour. This is even less of an issue as the options are now available through k6/execution and actually have the correct values instead of having some correct values.

Making any kind let's print something on this being abused will likely require quite a lot of code and I doubt it will have any effect -warnings are rarely being noticed unless they are obnoxious.

We will be writing it in the release notes, but if we have to slow down the development for ESM on the off chance people are abusing this - I am against it. It is still likely that they will have this next release without native ESM support and only this change.

This is also true for anything defined in the main module exported or not. It is just much easier to showcase with options particularly. This IMO should never have been the case and should be fixed. Additionally ESM in practice will "fix" this as modules all have their own scopes. So it makes sense to maybe break this before we add ESM support

codebien

LGTM, just a suggestion

js/bundle.go

olegbespalov · 2022-07-14T07:01:15Z

js/bundle.go

@@ -262,26 +263,31 @@ func (b *Bundle) Instantiate(logger logrus.FieldLogger, vuID uint64) (*BundleIns
 	}

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


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?

in all the places where access this we either:

know that there will be the key - otherwise we have some major bug

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.

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.

js/bundle.go

js/bundle_test.go

mstoykov · 2022-07-14T09:00:42Z

I will just squash this whole thing in the github interface at the end as I don't see why any of this should be in separate commits. If somebody wants something to remain as separate commits I am opened to it, it just doesn't seem necessary to me

olegbespalov

👍

na-- · 2022-07-14T14:29:00Z

js/bundle.go

 	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


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?

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

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.

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

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.

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 🤷

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

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?

I mean this:

k6/js/bundle.go

Lines 297 to 299 in 6b77bfc

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

instErr = err

}

This should work for all varitants 🤔

na-- · 2022-07-15T11:58:02Z

js/bundle.go

@@ -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()]


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.

na--

LGTM except the comment nitpick. Though I have to say, the JS initialization code is getting more complicated instead of less, and with ESM I expect the problem will only become worse? So I'd love some cleanup and renaming of confusingly named objects (Runner especially) soon, since it's difficult to keep track how everything fits together...

olegbespalov

👍

mstoykov requested review from oleiade, imiric, codebien, na-- and olegbespalov June 14, 2022 13:30

na-- added this to the v0.40.0 milestone Jun 15, 2022

na-- assigned mstoykov Jun 29, 2022

mstoykov force-pushed the wrapMainScriptAsWell branch from 36f9466 to b9bb3d0 Compare June 30, 2022 12:08

mstoykov mentioned this pull request Jun 30, 2022

Add to showcase how currently options is accessible everywhere #2566

Closed

mstoykov marked this pull request as ready for review June 30, 2022 12:09

mstoykov force-pushed the wrapMainScriptAsWell branch from b9bb3d0 to da13b03 Compare June 30, 2022 12:14

mstoykov changed the base branch from optionsAreGlobal to master July 7, 2022 11:51

mstoykov added 2 commits July 13, 2022 17:44

Contain main module within it's own scope as well

35146ca

mstoykov force-pushed the wrapMainScriptAsWell branch from da13b03 to 35146ca Compare July 13, 2022 14:49

codebien reviewed Jul 13, 2022

View reviewed changes

js/bundle.go Outdated Show resolved Hide resolved

olegbespalov reviewed Jul 14, 2022

View reviewed changes

mstoykov added 4 commits July 14, 2022 11:03

Drop TestBundleInstantiate/SetAndRun

42bf5d6

Better panic message

3e52cb8

Extract main commonjs module initialization

a614cfe

Improve compiler readability around commonjs wrapping parameters

6b77bfc

mstoykov requested review from codebien and olegbespalov July 14, 2022 08:59

codebien previously approved these changes Jul 14, 2022

View reviewed changes

olegbespalov previously approved these changes Jul 14, 2022

View reviewed changes

na-- added the breaking change for PRs that need to be mentioned in the breaking changes section of the release notes label Jul 14, 2022

na-- reviewed Jul 14, 2022

View reviewed changes

na-- mentioned this pull request Jul 14, 2022

Warn on access to the exported script options instead of test.options in k6/execution #2601

Open

na-- reviewed Jul 15, 2022

View reviewed changes

na-- previously approved these changes Jul 15, 2022

View reviewed changes

Add a comment about main script map lookup

7e289a5

mstoykov dismissed stale reviews from na--, olegbespalov, and codebien via 7e289a5 July 18, 2022 07:28

mstoykov requested review from codebien, na-- and olegbespalov July 18, 2022 07:29

na-- approved these changes Jul 18, 2022

View reviewed changes

codebien approved these changes Jul 18, 2022

View reviewed changes

olegbespalov approved these changes Jul 18, 2022

View reviewed changes

mstoykov merged commit e2316a2 into master Jul 18, 2022

mstoykov deleted the wrapMainScriptAsWell branch July 18, 2022 11:14

mstoykov mentioned this pull request Jul 29, 2022

Templating issue in latest k6 release #2623

Closed

na-- mentioned this pull request Aug 15, 2022

Function cannot read tag value from options.tags object directly #2278

Closed

codebien mentioned this pull request Aug 31, 2022

v0.40.0 release notes #2669

Merged

mstoykov mentioned this pull request Sep 12, 2022

Global variables declared in init context no longer work when running tests with docker #2680

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mstoykov commented Jun 14, 2022

codebien commented Jun 15, 2022

mstoykov commented Jun 30, 2022

codebien left a comment

olegbespalov Jul 14, 2022

mstoykov Jul 14, 2022

na-- Jul 15, 2022

mstoykov commented Jul 14, 2022

olegbespalov left a comment

na-- Jul 14, 2022

mstoykov Jul 14, 2022

codebien Jul 14, 2022 •

edited

Loading

na-- Jul 14, 2022 •

edited

Loading

mstoykov Jul 14, 2022

mstoykov Jul 15, 2022

na-- Jul 15, 2022

na-- Jul 15, 2022

na-- Jul 15, 2022 •

edited

Loading

mstoykov Jul 18, 2022

na-- Jul 15, 2022

na-- left a comment

olegbespalov left a comment

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

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

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

Conversation

mstoykov commented Jun 14, 2022

codebien commented Jun 15, 2022

mstoykov commented Jun 30, 2022

codebien 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

mstoykov commented Jul 14, 2022

olegbespalov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codebien Jul 14, 2022 • edited Loading

Choose a reason for hiding this comment

na-- Jul 14, 2022 • 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

na-- Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

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

olegbespalov left a comment

Choose a reason for hiding this comment

codebien Jul 14, 2022 •

edited

Loading

na-- Jul 14, 2022 •

edited

Loading

na-- Jul 15, 2022 •

edited

Loading