Cleanup comments and some code after ESM merge (#3862)

Co-authored-by: Oleg Bespalov <[email protected]>
Co-authored-by: Ivan <[email protected]>

cmd/runtime_options.go

@@ -24,9 +24,8 @@ func runtimeOptionFlagSet(includeSysEnv bool) *pflag.FlagSet {
 	flags.Bool("include-system-env-vars", includeSysEnv, "pass the real system environment variables to the runtime")
 	flags.String("compatibility-mode", "extended",
 		`JavaScript compiler compatibility mode, "extended" or "base" or "experimental_enhanced"
-base: pure Sobek - Golang JS VM supporting ES5.1+
-extended: base + Babel with parts of ES2015 preset
-		  slower to compile in case the script uses syntax unsupported by base
+base: pure Sobek - Golang JS VM supporting ES6+
+extended: base + sets "global" as alias for "globalThis"
 experimental_enhanced: esbuild-based transpiling for TypeScript and ES6+ support
 `)
 	flags.StringP("type", "t", "", "override test type, \"js\" or \"archive\"")

js/bundle.go

@@ -281,7 +281,6 @@ func (b *Bundle) newCompiler(logger logrus.FieldLogger) *compiler.Compiler {
 	c := compiler.New(logger)
 	c.Options = compiler.Options{
 		CompatibilityMode: b.CompatibilityMode,
-		Strict:            true,
 		SourceMapLoader:   generateSourceMapLoader(logger, b.filesystems),
 	}
 	return c

js/compiler/compiler.go

@@ -1,5 +1,5 @@
 // Package compiler implements additional functionality for k6 to compile js code.
-// more specifically transpiling through babel in case that is needed.
+// more specifically wrapping code in CommonJS wrapper or transforming it through esbuild for typescript support.
 // TODO this package name makes little sense now that it only parses and tranforms javascript.
 // Although people do call such transformation - compilation, so maybe it is still fine
 package compiler
@@ -18,10 +18,7 @@ import (
 	"go.k6.io/k6/lib"
 )
 
-// A Compiler compiles JavaScript source code (ES5.1 or ES6) into a sobek.Program
-const sourceMapURLFromBabel = "k6://internal-should-not-leak/file.map"
-
-// A Compiler compiles JavaScript source code (ES5.1 or ES6) into a sobek.Program
+// A Compiler compiles JavaScript or TypeScript source code into a sobek.Program
 type Compiler struct {
 	logger  logrus.FieldLogger
 	Options Options
@@ -36,19 +33,17 @@ func New(logger logrus.FieldLogger) *Compiler {
 type Options struct {
 	CompatibilityMode lib.CompatibilityMode
 	SourceMapLoader   func(string) ([]byte, error)
-	Strict            bool
 }
 
-// parsingState is helper struct to keep the state of a compilation
+// parsingState is helper struct to keep the state of a parsing
 type parsingState struct {
 	// set when we couldn't load external source map so we can try parsing without loading it
 	couldntLoadSourceMap bool
 	// srcMap is the current full sourceMap that has been generated read so far
 	srcMap            []byte
 	srcMapError       error
-	wrapped           bool // whether the original source is wrapped in a function to make it a commonjs module
+	commonJSWrapped   bool // whether the original source is wrapped in a function to make it a CommonJS module
 	compatibilityMode lib.CompatibilityMode
-	logger            logrus.FieldLogger
 	compiler          *Compiler
 
 	loader func(string) ([]byte, error)
@@ -58,50 +53,22 @@ type parsingState struct {
 // The returned program can be compiled directly by Sobek.
 // Additionally, it returns the end code that has been parsed including any required transformations.
 func (c *Compiler) Parse(
-	src, filename string, wrap bool,
+	src, filename string, commonJSWrap bool,
 ) (prg *ast.Program, finalCode string, err error) {
 	state := &parsingState{
 		loader:            c.Options.SourceMapLoader,
-		wrapped:           wrap,
 		compatibilityMode: c.Options.CompatibilityMode,
-		logger:            c.logger,
+		commonJSWrapped:   commonJSWrap,
 		compiler:          c,
 	}
-	return state.parseImpl(src, filename, wrap)
-}
-
-// sourceMapLoader is to be used with Sobek's WithSourceMapLoader
-// it not only gets the file from disk in the simple case, but also returns it if the map was generated from babel
-// additioanlly it fixes off by one error in commonjs dependencies due to having to wrap them in a function.
-func (ps *parsingState) sourceMapLoader(path string) ([]byte, error) {
-	if path == sourceMapURLFromBabel {
-		if ps.wrapped {
-			return ps.increaseMappingsByOne(ps.srcMap)
-		}
-		return ps.srcMap, nil
-	}
-	ps.srcMap, ps.srcMapError = ps.loader(path)
-	if ps.srcMapError != nil {
-		ps.couldntLoadSourceMap = true
-		return nil, ps.srcMapError
-	}
-	_, ps.srcMapError = sourcemap.Parse(path, ps.srcMap)
-	if ps.srcMapError != nil {
-		ps.couldntLoadSourceMap = true
-		ps.srcMap = nil
-		return nil, ps.srcMapError
-	}
-	if ps.wrapped {
-		return ps.increaseMappingsByOne(ps.srcMap)
-	}
-	return ps.srcMap, nil
+	return state.parseImpl(src, filename, commonJSWrap)
 }
 
-func (ps *parsingState) parseImpl(src, filename string, wrap bool) (*ast.Program, string, error) {
+func (ps *parsingState) parseImpl(src, filename string, commonJSWrap bool) (*ast.Program, string, error) {
 	code := src
-	if wrap { // the lines in the sourcemap (if available) will be fixed by increaseMappingsByOne
+	if commonJSWrap { // the lines in the sourcemap (if available) will be fixed by increaseMappingsByOne
 		code = ps.wrap(code, filename)
-		ps.wrapped = true
+		ps.commonJSWrapped = true
 	}
 	opts := parser.WithDisableSourceMaps
 	if ps.loader != nil {
@@ -113,7 +80,7 @@ func (ps *parsingState) parseImpl(src, filename string, wrap bool) (*ast.Program
 		ps.couldntLoadSourceMap = false // reset
 		// we probably don't want to abort scripts which have source maps but they can't be found,
 		// this also will be a breaking change, so if we couldn't we retry with it disabled
-		ps.logger.WithError(ps.srcMapError).Warnf("Couldn't load source map for %s", filename)
+		ps.compiler.logger.WithError(ps.srcMapError).Warnf("Couldn't load source map for %s", filename)
 		prg, err = parser.ParseFile(nil, filename, code, 0, parser.WithDisableSourceMaps, parser.IsModule)
 	}
 
@@ -128,11 +95,11 @@ func (ps *parsingState) parseImpl(src, filename string, wrap bool) (*ast.Program
 		}
 		if ps.loader != nil {
 			// This hack is required for the source map to work
-			code += "\n//# sourceMappingURL=" + sourceMapURLFromBabel
+			code += "\n//# sourceMappingURL=" + internalSourceMapURL
 		}
-		ps.wrapped = false
+		ps.commonJSWrapped = false
 		ps.compatibilityMode = lib.CompatibilityModeBase
-		return ps.parseImpl(code, filename, wrap)
+		return ps.parseImpl(code, filename, commonJSWrap)
 	}
 	return nil, "", err
 }
@@ -144,7 +111,7 @@ func (ps *parsingState) wrap(code, filename string) string {
 		conditionalNewLine = "\n"
 		newCode, err := ps.updateInlineSourceMap(code, index)
 		if err != nil {
-			ps.logger.Warnf("while compiling %q, couldn't update its inline sourcemap which might lead "+
+			ps.compiler.logger.Warnf("while parsing %q, couldn't update its inline sourcemap which might lead "+
 				"to some line numbers being off: %s", filename, err)
 		} else {
 			code = newCode
@@ -155,6 +122,36 @@ func (ps *parsingState) wrap(code, filename string) string {
 	return "(function(module, exports){" + conditionalNewLine + code + "\n})\n"
 }
 
+const internalSourceMapURL = "k6://internal-should-not-leak/file.map"
+
+// sourceMapLoader is to be used with Sobek's WithSourceMapLoader to add more than just loading files from disk.
+// It additionally:
+// - Loads a source-map if the it was generated from internal process.
+// - It additioanlly fixes off-by-one error for CommonJS dependencies due to having to wrap them in a functions.
+func (ps *parsingState) sourceMapLoader(path string) ([]byte, error) {
+	if path == internalSourceMapURL {
+		if ps.commonJSWrapped {
+			return ps.increaseMappingsByOne(ps.srcMap)
+		}
+		return ps.srcMap, nil
+	}
+	ps.srcMap, ps.srcMapError = ps.loader(path)
+	if ps.srcMapError != nil {
+		ps.couldntLoadSourceMap = true
+		return nil, ps.srcMapError
+	}
+	_, ps.srcMapError = sourcemap.Parse(path, ps.srcMap)
+	if ps.srcMapError != nil {
+		ps.couldntLoadSourceMap = true
+		ps.srcMap = nil
+		return nil, ps.srcMapError
+	}
+	if ps.commonJSWrapped {
+		return ps.increaseMappingsByOne(ps.srcMap)
+	}
+	return ps.srcMap, nil
+}
+
 func (ps *parsingState) updateInlineSourceMap(code string, index int) (string, error) {
 	nextnewline := strings.Index(code[index:], "\n")
 	if nextnewline == -1 {

js/compiler/compiler_test.go

@@ -118,7 +118,6 @@ func TestCorruptSourceMap(t *testing.T) {
 
 	compiler := New(logger)
 	compiler.Options = Options{
-		Strict: true,
 		SourceMapLoader: func(string) ([]byte, error) {
 			return corruptSourceMap, nil
 		},
@@ -134,41 +133,8 @@ func TestCorruptSourceMap(t *testing.T) {
 	require.Contains(t, msg, `json: cannot unmarshal number into Go struct field v3.mappings of type string`)
 }
 
-func TestCorruptSourceMapOnlyForBabel(t *testing.T) {
-	t.Parallel()
-	// This test is now kind of pointless
-	// this a valid source map for the go implementation but babel doesn't like it
-	corruptSourceMap := []byte(`{"mappings": ";"}`)
-
-	logger := logrus.New()
-	logger.SetLevel(logrus.DebugLevel)
-	logger.Out = io.Discard
-	hook := testutils.SimpleLogrusHook{
-		HookedLevels: []logrus.Level{logrus.InfoLevel, logrus.WarnLevel},
-	}
-	logger.AddHook(&hook)
-
-	compiler := New(logger)
-	compiler.Options = Options{
-		CompatibilityMode: lib.CompatibilityModeExtended,
-		Strict:            true,
-		SourceMapLoader: func(string) ([]byte, error) {
-			return corruptSourceMap, nil
-		},
-	}
-	prg, _, err := compiler.Parse("import 'something';\n//# sourceMappingURL=somefile", "somefile", false)
-	require.NoError(t, err)
-	_, err = sobek.CompileAST(prg, true)
-	require.NoError(t, err)
-	require.Empty(t, hook.Drain())
-	_, err = sobek.CompileAST(prg, true)
-	require.NoError(t, err)
-	require.Empty(t, hook.Drain())
-}
-
 func TestMinimalSourceMap(t *testing.T) {
 	t.Parallel()
-	// this is the minimal sourcemap valid for both go and babel implementations
 	corruptSourceMap := []byte(`{"version":3,"mappings":";","sources":[]}`)
 
 	logger := logrus.New()
@@ -182,7 +148,6 @@ func TestMinimalSourceMap(t *testing.T) {
 	compiler := New(logger)
 	compiler.Options = Options{
 		CompatibilityMode: lib.CompatibilityModeExtended,
-		Strict:            true,
 		SourceMapLoader: func(string) ([]byte, error) {
 			return corruptSourceMap, nil
 		},

js/initcontext_test.go

@@ -631,69 +631,6 @@ export default function () {
 	require.Equal(t, "cool is cool\n\tat webpack:///./test1.ts:2:4(2)\n\tat webpack:///./test1.ts:5:4(3)\n\tat default (file:///script.js:4:4(3))\n", exception.String())
 }
 
-func TestSourceMapsExternalExtented(t *testing.T) {
-	t.Parallel()
-	fs := fsext.NewMemMapFs()
-	// This example is created through the template-typescript
-	// but was exported to use import/export syntax so it has to go through babel
-	require.NoError(t, fsext.WriteFile(fs, "/test1.js", []byte(`
-var o={d:(e,r)=>{for(var t in r)o.o(r,t)&&!o.o(e,t)&&Object.defineProperty(e,t,{enumerable:!0,get:r[t]})},o:(o,e)=>Object.prototype.hasOwnProperty.call(o,e)},e={};o.d(e,{Z:()=>r});const r=()=>{!function(o){throw"cool is cool"}()};var t=e.Z;export{t as default};
-//# sourceMappingURL=test1.js.map
-`[1:]), 0o644))
-	require.NoError(t, fsext.WriteFile(fs, "/test1.js.map", []byte(`
-{"version":3,"sources":["webpack:///webpack/bootstrap","webpack:///webpack/runtime/define property getters","webpack:///webpack/runtime/hasOwnProperty shorthand","webpack:///./test1.ts"],"names":["__webpack_require__","exports","definition","key","o","Object","defineProperty","enumerable","get","obj","prop","prototype","hasOwnProperty","call","s","coolThrow"],"mappings":"AACA,IAAIA,EAAsB,CCA1B,EAAwB,CAACC,EAASC,KACjC,IAAI,IAAIC,KAAOD,EACXF,EAAoBI,EAAEF,EAAYC,KAASH,EAAoBI,EAAEH,EAASE,IAC5EE,OAAOC,eAAeL,EAASE,EAAK,CAAEI,YAAY,EAAMC,IAAKN,EAAWC,MCJ3E,EAAwB,CAACM,EAAKC,IAAUL,OAAOM,UAAUC,eAAeC,KAAKJ,EAAKC,I,sBCGlF,cAHA,SAAmBI,GACf,KAAM,eAGNC,I","file":"test1.js","sourcesContent":["// The require scope\nvar __webpack_require__ = {};\n\n","// define getter functions for harmony exports\n__webpack_require__.d = (exports, definition) => {\n\tfor(var key in definition) {\n\t\tif(__webpack_require__.o(definition, key) && !__webpack_require__.o(exports, key)) {\n\t\t\tObject.defineProperty(exports, key, { enumerable: true, get: definition[key] });\n\t\t}\n\t}\n};","__webpack_require__.o = (obj, prop) => (Object.prototype.hasOwnProperty.call(obj, prop))","function coolThrow(s: string) {\n    throw \"cool \"+ s\n}\nexport default () => {\n    coolThrow(\"is cool\")\n};\n"],"sourceRoot":""}
-`[1:]), 0o644))
-	data := `
-import l from "./test1.js"
-
-export default function () {
-		l()
-};
-`[1:]
-	b, err := getSimpleBundle(t, "/script.js", data, fs)
-	require.NoError(t, err)
-
-	bi, err := b.Instantiate(context.Background(), 0)
-	require.NoError(t, err)
-	_, err = bi.getCallableExport(consts.DefaultFn)(sobek.Undefined())
-	require.Error(t, err)
-	exception := new(sobek.Exception)
-	require.ErrorAs(t, err, &exception)
-	// TODO figure out why those are not the same as the one in the previous test TestSourceMapsExternal
-	// likely settings in the transpilers
-	require.Equal(t, "cool is cool\n\tat webpack:///./test1.ts:2:4(2)\n\tat r (webpack:///./test1.ts:5:4(3))\n\tat default (file:///script.js:4:4(3))\n", exception.String())
-}
-
-func TestSourceMapsExternalExtentedInlined(t *testing.T) {
-	t.Parallel()
-	fs := fsext.NewMemMapFs()
-	// This example is created through the template-typescript
-	// but was exported to use import/export syntax so it has to go through babel
-	require.NoError(t, fsext.WriteFile(fs, "/test1.js", []byte(`
-var o={d:(e,r)=>{for(var t in r)o.o(r,t)&&!o.o(e,t)&&Object.defineProperty(e,t,{enumerable:!0,get:r[t]})},o:(o,e)=>Object.prototype.hasOwnProperty.call(o,e)},e={};o.d(e,{Z:()=>r});const r=()=>{!function(o){throw"cool is cool"}()};var t=e.Z;export{t as default};
-//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIndlYnBhY2s6Ly8vd2VicGFjay9ib290c3RyYXAiLCJ3ZWJwYWNrOi8vL3dlYnBhY2svcnVudGltZS9kZWZpbmUgcHJvcGVydHkgZ2V0dGVycyIsIndlYnBhY2s6Ly8vd2VicGFjay9ydW50aW1lL2hhc093blByb3BlcnR5IHNob3J0aGFuZCIsIndlYnBhY2s6Ly8vLi90ZXN0MS50cyJdLCJuYW1lcyI6WyJfX3dlYnBhY2tfcmVxdWlyZV9fIiwiZXhwb3J0cyIsImRlZmluaXRpb24iLCJrZXkiLCJvIiwiT2JqZWN0IiwiZGVmaW5lUHJvcGVydHkiLCJlbnVtZXJhYmxlIiwiZ2V0Iiwib2JqIiwicHJvcCIsInByb3RvdHlwZSIsImhhc093blByb3BlcnR5IiwiY2FsbCIsInMiLCJjb29sVGhyb3ciXSwibWFwcGluZ3MiOiJBQUNBLElBQUlBLEVBQXNCLENDQTFCLEVBQXdCLENBQUNDLEVBQVNDLEtBQ2pDLElBQUksSUFBSUMsS0FBT0QsRUFDWEYsRUFBb0JJLEVBQUVGLEVBQVlDLEtBQVNILEVBQW9CSSxFQUFFSCxFQUFTRSxJQUM1RUUsT0FBT0MsZUFBZUwsRUFBU0UsRUFBSyxDQUFFSSxZQUFZLEVBQU1DLElBQUtOLEVBQVdDLE1DSjNFLEVBQXdCLENBQUNNLEVBQUtDLElBQVVMLE9BQU9NLFVBQVVDLGVBQWVDLEtBQUtKLEVBQUtDLEksc0JDR2xGLGNBSEEsU0FBbUJJLEdBQ2YsS0FBTSxlQUdOQyxJIiwiZmlsZSI6InRlc3QxLmpzIiwic291cmNlc0NvbnRlbnQiOlsiLy8gVGhlIHJlcXVpcmUgc2NvcGVcbnZhciBfX3dlYnBhY2tfcmVxdWlyZV9fID0ge307XG5cbiIsIi8vIGRlZmluZSBnZXR0ZXIgZnVuY3Rpb25zIGZvciBoYXJtb255IGV4cG9ydHNcbl9fd2VicGFja19yZXF1aXJlX18uZCA9IChleHBvcnRzLCBkZWZpbml0aW9uKSA9PiB7XG5cdGZvcih2YXIga2V5IGluIGRlZmluaXRpb24pIHtcblx0XHRpZihfX3dlYnBhY2tfcmVxdWlyZV9fLm8oZGVmaW5pdGlvbiwga2V5KSAmJiAhX193ZWJwYWNrX3JlcXVpcmVfXy5vKGV4cG9ydHMsIGtleSkpIHtcblx0XHRcdE9iamVjdC5kZWZpbmVQcm9wZXJ0eShleHBvcnRzLCBrZXksIHsgZW51bWVyYWJsZTogdHJ1ZSwgZ2V0OiBkZWZpbml0aW9uW2tleV0gfSk7XG5cdFx0fVxuXHR9XG59OyIsIl9fd2VicGFja19yZXF1aXJlX18ubyA9IChvYmosIHByb3ApID0+IChPYmplY3QucHJvdG90eXBlLmhhc093blByb3BlcnR5LmNhbGwob2JqLCBwcm9wKSkiLCJmdW5jdGlvbiBjb29sVGhyb3coczogc3RyaW5nKSB7XG4gICAgdGhyb3cgXCJjb29sIFwiKyBzXG59XG5leHBvcnQgZGVmYXVsdCAoKSA9PiB7XG4gICAgY29vbFRocm93KFwiaXMgY29vbFwiKVxufTtcbiJdLCJzb3VyY2VSb290IjoiIn0=
-`[1:]), 0o644))
-	data := `
-import l from "./test1.js"
-
-export default function () {
-		l()
-};
-`[1:]
-	b, err := getSimpleBundle(t, "/script.js", data, fs)
-	require.NoError(t, err)
-
-	bi, err := b.Instantiate(context.Background(), 0)
-	require.NoError(t, err)
-	_, err = bi.getCallableExport(consts.DefaultFn)(sobek.Undefined())
-	require.Error(t, err)
-	exception := new(sobek.Exception)
-	require.ErrorAs(t, err, &exception)
-	// TODO figure out why those are not the same as the one in the previous test TestSourceMapsExternal
-	// likely settings in the transpilers
-	require.Equal(t, "cool is cool\n\tat webpack:///./test1.ts:2:4(2)\n\tat r (webpack:///./test1.ts:5:4(3))\n\tat default (file:///script.js:4:4(3))\n", exception.String())
-}
-
 func TestSourceMapsInlinedCJS(t *testing.T) {
 	t.Parallel()
 	fs := fsext.NewMemMapFs()

js/tc39/README.md

@@ -1,14 +1,14 @@
 # Introduction to a k6's TC39 testing
 
-The point of this module is to test k6 Sobek+babel and k6 Sobek+esbuild combo against the tc39 test suite.
+`js/tc39` package tests k6 [Sobek](https://github.com/grafana/sobek) and the k6 Sobek+esbuild combo against the tc39 test suite.
 
 Ways to use it:
 1. run ./checkout.sh to checkout the last commit sha of [test262](https://github.com/tc39/test262)
    that was tested with this module
 2. Run `go test &> out.log`
 
 The full list of failing tests, and the error, is in `breaking_test_errors-*.json`. All errors list there with the corresponding error will *not* be counted as errors - this is what the test expects, those specific errors.
-Due to changes to soben it is not uncommon for the error to change, or there to be now a new error on previously passing test, or (hopefully) a test that was not passing but now is.
+Due to changes to Sobek it's common for the error to change, or there to be now a new error on the previously passing test, or (hopefully) a test that was not passing but now is.
 In all of those cases `breaking_test_errors-*.json` needs to be updated. Run the test with `-update` flag to update: `go test -update`
 
 NOTE: some text editors/IDEs will try to parse files ending in `json` as JSON, which given the size of `breaking_test_errors-*.json` might be a problem when it's not actually a JSON (before the edit). So it might be a better idea to name it something different if editing by hand and fix it later.
@@ -27,6 +27,6 @@ This also means that, if the
 previous breakage was something a user can work around in a certain way, it now might be something
 else that the user can't workaround or have another problem.
 
-On top of that this more or less exist as k6 add babel to the mix and that changes which tests pass and how some of them fail. When we remove babel, it's very likely we will also remove this package as it won't be relevant anymore.
+Starting v0.53 k6 doesn't use Babel anymore, it now serves tests for `esbuild`, and for the parts uncovered by Sobek's test suite. It is still possible that we drop the whole package in the future.
 
 For these reasons, I decided that recording what breaks and checking that it doesn't change is better.

lib/runtime_options.go

@@ -13,9 +13,9 @@ import (
 type CompatibilityMode uint8
 
 const (
-	// CompatibilityModeExtended achieves ES6+ compatibility with Babel
+	// CompatibilityModeExtended adds `global` as an alias for `globalThis` on top of the base mode
 	CompatibilityModeExtended CompatibilityMode = iota + 1
-	// CompatibilityModeBase is standard Sobek ES5.1+
+	// CompatibilityModeBase is standard Sobek, which means pure vanilla JS following ECMAScript standards.
 	CompatibilityModeBase
 	// CompatibilityModeExperimentalEnhanced achieves TypeScript and ES6+ compatibility with esbuild
 	CompatibilityModeExperimentalEnhanced
@@ -28,7 +28,7 @@ type RuntimeOptions struct {
 	// Whether to pass the actual system environment variables to the JS runtime
 	IncludeSystemEnvVars null.Bool `json:"includeSystemEnvVars"`
 
-	// JS compatibility mode: "extended" (Goja+Babel) or "base" (plain Goja)
+	// JS compatibility mode: "extended" (Sobek+global) or "base" (plain Sobek)
 	//
 	// TODO: when we resolve https://github.com/k6io/k6/issues/883, we probably
 	// should use the CompatibilityMode type directly... but by then, we'd need to have