Add JS compatibility mode option

[imiric]

Closes: #1049

This adds a new CLI run --compatibility-mode flag and K6_COMPATIBILITY_MODE environment variable whose values can be:

• "extended": this is the default and current compatibility mode, which uses Babel and core.js to achieve ES6+ compatibility.
• "base": an optional mode that disables loading of Babel and core.js, running scripts with only Goja's native ES5.1+ compatibility. If the test scripts don't require ES6 compatibility (e.g. they were previously transformed by Babel), this option can be used to reduce RAM usage during test runs.

I used the following test script to test ES 5.1 runs:

var http = require("k6/http");
var metrics = require("k6/metrics");

var myTrend = new metrics.Trend("my_trend");

function measure(fn) {
    var start = Date.now();
    var res = fn();
    var end = Date.now();
    console.log("Duration: ", end-start);
    myTrend.add(end - start);
    return res;
}

function run() {
    measure(function() {
        http.get("https://testuser1:testpwd1@httpbingo.org/basic-auth/testuser1/testpwd1")
    });
    measure(function() {
        http.get("https://testuser2:testpwd2@httpbingo.org/digest-auth/auth/testuser2/testpwd2", { auth: "digest" })
    })
}

module.exports.default = run;

I didn't do any performance comparisons, but those are summed up nicely in #1167.

[codecov-io]

Codecov Report

Merging #1206 into master will increase coverage by 0.02%.
The diff coverage is 80.57%.

@@            Coverage Diff             @@
##           master    #1206      +/-   ##
==========================================
+ Coverage   75.31%   75.34%   +0.02%     
==========================================
  Files         147      148       +1     
  Lines       10711    10763      +52     
==========================================
+ Hits         8067     8109      +42     
- Misses       2180     2189       +9     
- Partials      464      465       +1

Impacted Files	Coverage Δ
lib/archive.go	85.95% <ø> (ø)	⬆️
js/common/util.go	100% <100%> (+16.66%)	⬆️
js/initcontext.go	90.21% <100%> (+0.32%)	⬆️
js/compiler/compatibility_mode_gen.go	56.25% <56.25%> (ø)
js/compiler/compiler.go	78.94% <76%> (-0.3%)	⬇️
cmd/runtime_options.go	90.24% <80%> (-3.31%)	⬇️
lib/runtime_options.go	55.55% <83.33%> (+55.55%)	⬆️
js/bundle.go	85.18% <89.18%> (+2.5%)	⬆️
stats/cloud/collector.go	76.99% <0%> (-0.62%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c73438...3cd1799. Read the comment docs.

[mstoykov]

Definitely add tests that some code works in es51 and that some code doesn't work in es51 but works in es6
I was going to try to look in how to hack around the configuration problems, but got a headache ... so, maybe on a future review will try again :). Hopefully @na-- suggests something without getting a headache as well

[cuonglm]

Same as https://github.com/loadimpact/k6/pull/1206/files#diff-46cbc2690f760ed10e27cdb22cd67c10R65

[imiric]

I'm also actually not disabling core.js yet 😄:

https://github.com/loadimpact/k6/blob/9a34e249828a86bd46586de3a2e8bfb552e26033/js/bundle.go#L255-L257

Will fix/test.

[imiric]

So it turns out not to be trivial implementing tests for this.

The current failures are because essentially Compiler is a singleton, which is why it's created with once.Do(), and the purpose of the mutex.

(You can confirm it's because of this since they pass in isolation: go test -v -race -run 'TestNewBundle/CompatibilityModeES51' ./js)

The problem with this is that up until now we haven't had the need for Compiler instance data to change, but for compatibility mode tests we do. I do think compatibilityMode should be a field on Compiler, but we could also pass it to Compiler.Compile (in addition to compiler.New) which would fix this at the cost of repetition.

I tried setting compatibilityMode outside of once.Do() behind the mutex but still got race condition errors, and I don't think this should be worked around like that.

Let me know what you think is the best way to proceed here. I think passing the compatibility mode also to Compiler.Compile is the easiest route.

[imiric]

So this required a substantial refactoring of compiler.go, pretty much to make it testable, as the functionality worked when invoked in the CLI (as there was only one instance of Compiler), but I think it resulted in a much cleaner implementation.

The change was to make a new compiler.babel struct a singleton, instead of compiler.Compiler, since the value of compatibilityMode (previously checked in new()) needed to change between tests.

I think this abstracts nicely the parts that need to remain a singleton (the Babel Goja VM and the Babel object itself), while allowing reuse of Compiler for different compilation modes.

Let me know what you think, and if the configuration/validation can be improved in this PR (how?) or if it can wait for #883. Also if more tests should be added or if these baseline ones are enough. (I should probably add core.js tests...)

[imiric]

This test was lying, well, still is. :) It's part of the "ES5" suite, yet this error is returned by Babel, not Goja (which returns a less descriptive script.js: Line 1:27 Unexpected token ) (and 2 more errors)).

Should I just update the assert to reflect that? Right now I'm passing CompatibilityModeExtended to make it pass.

[imiric]

To continue our discussion from last Friday re: Compiler, I did an experimental redesign of what I meant by wanting for Compiler to also compile core.js.

Take a look at 4e52c78 (it's built on this branch).

There's a few reasons I think this design is better:

• It's more cohesive for Compile, to, well, compile everything that's needed for the VU runtime, instead of this being done in Bundle.instantiate. There shouldn't be any major performance difference either way.
• It introduces the concept of "preprocessor" and a place to expand with internal or other shims.
• It gets rid of the additional once.Do() call and the entire js/lib package, so negative SLOC! (Mostly because of rice-box and the license header, but still...)

Initially I wanted to modify the main goja.Program or ast.Program (and avoid the additional PreProgram variable) to inject any shims into the object returned by Compiler.Compile, but decided against this as a) Goja internals scare me, and b) I didn't see an easy MergeASTs method, and doing it manually would probably alter the source maps, which I don't want to hack around with.

Let me know if you think this is an improvement or not, and if so, if it should be part of this PR or left for after v0.26.0.

[mstoykov]

wrong PR 🤦‍♂️

[mstoykov]

LGTM

[na--]

As a nit to fix in the future (i.e. #883), I'm wondering if the actual CompatibilityMode type shouldn't be defined somewhere low-level (i.e. lib or wherever we decide to put these foundational types) and then instead of using a string here, we could just use that type (or some null-able wrapper of it).

Because right now, the stringy type here isn't very convenient. Even though it has been validated before we ever use it, we kind of have to cast it to the actual type if we ever want to use it, and then deal with the error that's never going to come...