Skip to content

Commit

Permalink
Drop legacy require behavior support
Browse files Browse the repository at this point in the history 
Closes #3534
  • Loading branch information
@mstoykov
mstoykov committed Jul 15, 2024
1 parent 5d4c902 commit 0c6d9f6
Show file tree
Hide file tree
Showing 5 changed files with 92 additions and 80 deletions.
8 changes: 4 additions & 4 deletions js/bundle.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
Expand All @@ -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
}
Expand Down
66 changes: 9 additions & 57 deletions js/modules/require_impl.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(wm)
gmi, err = ms.getModuleInstanceFromGoModule(wm)
if err != nil {
return nil, err
}
Expand All @@ -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(fmt.Sprintf("expected sobek.CyclicModuleRecord, but for some reason got a %T", m))
}
Expand Down Expand Up @@ -137,8 +89,8 @@ func (ms *ModuleSystem) getModuleInstanceFromGoModule(wm *goModule) (wmi *goModu

// 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
}
Expand Down
3 changes: 1 addition & 2 deletions js/modulestest/runtime.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
92 changes: 78 additions & 14 deletions js/path_resolution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down Expand Up @@ -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")();
Expand All @@ -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": `
Expand All @@ -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": `
Expand All @@ -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 {
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
3 changes: 0 additions & 3 deletions js/tc39/tc39_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 0c6d9f6

Please sign in to comment.