diff --git a/js/bundle.go b/js/bundle.go index 561649c97db..821b42acf03 100644 --- a/js/bundle.go +++ b/js/bundle.go @@ -396,14 +396,14 @@ func (b *Bundle) setupJSRuntime(rt *sobek.Runtime, vuID int64, logger logrus.Fie // this exists only to make the check in the init context. type requireImpl struct { inInitContext func() bool - internal *modules.LegacyRequireImpl + modSys *modules.ModuleSystem } func (r *requireImpl) require(specifier string) (*sobek.Object, error) { if !r.inInitContext() { return nil, fmt.Errorf(cantBeUsedOutsideInitContextMsg, "require") } - return r.internal.Require(specifier) + return r.modSys.Require(specifier) } func (b *Bundle) setInitGlobals(rt *sobek.Runtime, vu *moduleVUImpl, modSys *modules.ModuleSystem) { @@ -415,7 +415,7 @@ func (b *Bundle) setInitGlobals(rt *sobek.Runtime, vu *moduleVUImpl, modSys *mod impl := requireImpl{ inInitContext: func() bool { return vu.state == nil }, - internal: modules.NewLegacyRequireImpl(vu, modSys), + modSys: modSys, } mustSet("require", impl.require) @@ -430,7 +430,7 @@ func (b *Bundle) setInitGlobals(rt *sobek.Runtime, vu *moduleVUImpl, modSys *mod return nil, errors.New("open() can't be used with an empty filename") } // This uses the pwd from the requireImpl - pwd, err := impl.internal.CurrentlyRequiredModule() + pwd, err := modSys.CurrentlyRequiredModule() if err != nil { return nil, err } diff --git a/js/modules/require_impl.go b/js/modules/require_impl.go index 55c2d0c29f1..8f6a94a2101 100644 --- a/js/modules/require_impl.go +++ b/js/modules/require_impl.go @@ -10,71 +10,23 @@ import ( "go.k6.io/k6/loader" ) -// LegacyRequireImpl is a legacy implementation of `require()` that is not compatible with -// CommonJS as it loads modules relative to the currently required file, -// instead of relative to the file the `require()` is written in. -// See https://github.com/grafana/k6/issues/2674 -type LegacyRequireImpl struct { - vu VU - modules *ModuleSystem -} - -// NewLegacyRequireImpl creates a new LegacyRequireImpl -func NewLegacyRequireImpl(vu VU, ms *ModuleSystem) *LegacyRequireImpl { - return &LegacyRequireImpl{ - vu: vu, - modules: ms, - } -} - -const issueLink = "https://github.com/grafana/k6/issues/3534" - -func (r *LegacyRequireImpl) warnUserOnPathResolutionDifferences(specifier, parentModuleStr, parentModuleStr2 string) { - if r.modules.resolver.locked { - return - } - normalizePathToURL := func(path string) string { - u, err := url.Parse(path) - if err != nil { - return path - } - return loader.Dir(u).String() - } - parentModuleStrDir := normalizePathToURL(parentModuleStr) - parentModuleStr2Dir := normalizePathToURL(parentModuleStr2) - if parentModuleStr != parentModuleStr2 { - r.vu.InitEnv().Logger.Warnf( - `The "wrong" path (%q) and the path actually used by k6 (%q) to resolve %q are different. `+ - `This will break in the future please see %s.`, - parentModuleStrDir, parentModuleStr2Dir, specifier, issueLink) - } -} - // Require is the actual call that implements require -func (r *LegacyRequireImpl) Require(specifier string) (*sobek.Object, error) { +func (ms *ModuleSystem) Require(specifier string) (*sobek.Object, error) { if specifier == "" { return nil, errors.New("require() can't be used with an empty specifier") } - rt := r.vu.Runtime() - parentModuleStr := getCurrentModuleScript(r.vu) - parentModuleStr2, err := getPreviousRequiringFile(r.vu) - if err != nil { - return nil, err - } - if parentModuleStr != parentModuleStr2 { - r.warnUserOnPathResolutionDifferences(specifier, parentModuleStr, parentModuleStr2) - parentModuleStr = parentModuleStr2 - } + rt := ms.vu.Runtime() + parentModuleStr := getCurrentModuleScript(ms.vu) - parentModule, _ := r.modules.resolver.sobekModuleResolver(nil, parentModuleStr) - m, err := r.modules.resolver.sobekModuleResolver(parentModule, specifier) + parentModule, _ := ms.resolver.sobekModuleResolver(nil, parentModuleStr) + m, err := ms.resolver.sobekModuleResolver(parentModule, specifier) if err != nil { return nil, err } if wm, ok := m.(*goModule); ok { var gmi *goModuleInstance - gmi, err = r.modules.getModuleInstanceFromGoModule(rt, wm) + gmi, err = ms.getModuleInstanceFromGoModule(rt, wm) if err != nil { return nil, err } @@ -87,7 +39,7 @@ func (r *LegacyRequireImpl) Require(specifier string) (*sobek.Object, error) { } var promise *sobek.Promise if c, ok := m.(sobek.CyclicModuleRecord); ok { - promise = rt.CyclicModuleRecordEvaluate(c, r.modules.resolver.sobekModuleResolver) + promise = rt.CyclicModuleRecordEvaluate(c, ms.resolver.sobekModuleResolver) } else { panic("shouldn't happen") } @@ -137,8 +89,8 @@ func (ms *ModuleSystem) getModuleInstanceFromGoModule( // CurrentlyRequiredModule returns the module that is currently being required. // It is mostly used for old and somewhat buggy behaviour of the `open` call -func (r *LegacyRequireImpl) CurrentlyRequiredModule() (*url.URL, error) { - fileStr, err := getPreviousRequiringFile(r.vu) +func (ms *ModuleSystem) CurrentlyRequiredModule() (*url.URL, error) { + fileStr, err := getPreviousRequiringFile(ms.vu) if err != nil { return nil, err } diff --git a/js/modulestest/runtime.go b/js/modulestest/runtime.go index 005d9ab744b..2738e9b3762 100644 --- a/js/modulestest/runtime.go +++ b/js/modulestest/runtime.go @@ -114,7 +114,6 @@ func (r *Runtime) RunOnEventLoop(code string) (value sobek.Value, err error) { func (r *Runtime) innerSetupModuleSystem() error { ms := modules.NewModuleSystem(r.mr, r.VU) - impl := modules.NewLegacyRequireImpl(r.VU, ms) modules.ExportGloballyModule(r.VU.RuntimeField, ms, "k6/timers") - return r.VU.RuntimeField.Set("require", impl.Require) + return r.VU.RuntimeField.Set("require", ms.Require) } diff --git a/js/path_resolution_test.go b/js/path_resolution_test.go index 24200fefe48..6fa224a5149 100644 --- a/js/path_resolution_test.go +++ b/js/path_resolution_test.go @@ -106,8 +106,9 @@ func TestOpenPathResolution(t *testing.T) { func TestRequirePathResolution(t *testing.T) { t.Parallel() testCases := map[string]struct { - fsMap map[string]any - expectedLogs []string + fsMap map[string]any + expectedLogs []string + expectedError string }{ "simple": { fsMap: map[string]any{ @@ -140,8 +141,7 @@ func TestRequirePathResolution(t *testing.T) { fsMap: map[string]any{ "/A/B/data.js": "module.exports='export content'", "/A/C/B/script.js": ` - // Here the path is relative to this module but to the one calling - module.exports = () => require("./../data.js"); + module.exports = () => require("./../../B/data.js"); `, "/A/B/B/script.js": ` module.exports = require("./../../C/B/script.js")(); @@ -154,17 +154,33 @@ func TestRequirePathResolution(t *testing.T) { export default function() {} `, }, - expectedLogs: []string{ - `The "wrong" path ("file:///A/C/B/") and the path actually used by k6 ("file:///A/B/B/") to resolve "./../data.js" are different`, + }, + "complex wrong": { + fsMap: map[string]any{ + "/A/B/data.js": "module.exports='export content'", + "/A/C/B/script.js": ` + module.exports = () => require("./../data.js"); + `, + "/A/B/B/script.js": ` + module.exports = require("./../../C/B/script.js")(); + `, + "/A/A/A/A/script.js": ` + let data = require("./../../../B/B/script.js"); + if (data != "export content") { + throw new Error("wrong content " + data); + } + export default function() {} + `, }, + expectedError: `The moduleSpecifier "./../data.js" couldn't be found on local disk.`, }, "ESM and require": { fsMap: map[string]any{ "/A/B/data.js": "module.exports='export content'", "/A/C/B/script.js": ` export default function () { - // Here the path is relative to this module but to the one calling - return require("./../data.js"); + // Here the path is relative to this module not the calling one + return require("./../../B/data.js"); } `, "/A/B/B/script.js": ` @@ -179,17 +195,36 @@ func TestRequirePathResolution(t *testing.T) { export default function() {} `, }, - expectedLogs: []string{ - `The "wrong" path ("file:///A/C/B/") and the path actually used by k6 ("file:///A/B/B/") to resolve "./../data.js" are different`, + }, + "ESM and require wrong": { + fsMap: map[string]any{ + "/A/B/data.js": "module.exports='export content'", + "/A/C/B/script.js": ` + export default function () { + return require("./../data.js"); + } + `, + "/A/B/B/script.js": ` + import s from "./../../C/B/script.js" + export default require("./../../C/B/script.js").default(); + `, + "/A/A/A/A/script.js": ` + import data from "./../../../B/B/script.js" + if (data != "export content") { + throw new Error("wrong content " + data); + } + export default function() {} + `, }, + expectedError: `The moduleSpecifier "./../data.js" couldn't be found on local disk.`, }, "full ESM": { fsMap: map[string]any{ "/A/B/data.js": "export default 'export content'", "/A/C/B/script.js": ` export default function () { - // Here the path is relative to this module but to the one calling - return require("./../data.js").default; + // Here the path is relative to this module not the calling one + return require("./../../B/data.js").default; } `, "/A/B/B/script.js": ` @@ -205,9 +240,29 @@ func TestRequirePathResolution(t *testing.T) { export default function() {} `, }, - expectedLogs: []string{ - `The "wrong" path ("file:///A/C/B/") and the path actually used by k6 ("file:///A/B/B/") to resolve "./../data.js" are different`, + }, + "full ESM wrong": { + fsMap: map[string]any{ + "/A/B/data.js": "export default 'export content'", + "/A/C/B/script.js": ` + export default function () { + return require("./../data.js").default; + } + `, + "/A/B/B/script.js": ` + import s from "./../../C/B/script.js" + let l = s(); + export default l; + `, + "/A/A/A/A/script.js": ` + import data from "./../../../B/B/script.js" + if (data != "export content") { + throw new Error("wrong content " + data); + } + export default function() {} + `, }, + expectedError: `The moduleSpecifier "./../data.js" couldn't be found on local disk.`, }, } for name, testCase := range testCases { @@ -221,6 +276,11 @@ func TestRequirePathResolution(t *testing.T) { require.NoError(t, err) logger, hook := testutils.NewLoggerWithHook(t, logrus.WarnLevel) b, err := getSimpleBundle(t, "/main.js", `export { default } from "/A/A/A/A/script.js"`, fs, logger) + + if testCase.expectedError != "" { + require.ErrorContains(t, err, testCase.expectedError) + return + } require.NoError(t, err) _, err = b.Instantiate(context.Background(), 0) @@ -254,6 +314,10 @@ func TestRequirePathResolution(t *testing.T) { logger, hook := testutils.NewLoggerWithHook(t, logrus.WarnLevel) b, err := getSimpleBundleStdin(t, pwd, testCase.fsMap["/A/A/A/A/script.js"].(string), fs, logger) + if testCase.expectedError != "" { + require.ErrorContains(t, err, testCase.expectedError) + return + } require.NoError(t, err) _, err = b.Instantiate(context.Background(), 0) diff --git a/js/tc39/tc39_test.go b/js/tc39/tc39_test.go index f8018b598c8..1db4d4a4c2e 100644 --- a/js/tc39/tc39_test.go +++ b/js/tc39/tc39_test.go @@ -24,7 +24,6 @@ import ( "github.com/grafana/sobek" "github.com/grafana/sobek/parser" "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" "go.k6.io/k6/js/compiler" "go.k6.io/k6/js/modules" "go.k6.io/k6/js/modulestest" @@ -695,8 +694,6 @@ func (ctx *tc39TestCtx) runTC39Module(name, src string, includes []string, vm *s ctx.compiler(), base) ms := modules.NewModuleSystem(mr, moduleRuntime.VU) - impl := modules.NewLegacyRequireImpl(moduleRuntime.VU, ms) - require.NoError(ctx.t, vm.Set("require", impl.Require)) moduleRuntime.VU.InitEnvField.CWD = base early = false