Skip to content

Commit

Permalink
breaking change: Don't try to prepend https to module specifiers
Browse files Browse the repository at this point in the history 
This has been deprecated for years and never really made much sense, but
made a bunch of the error messages way more convoluted.

Closes #3287

This also fixes tc39 tests and some archive ones that were depending on
this functionality.

AFAIK all of those tests should've not been written the way they were to
begin with - they just happened to work due to the previous behaviour.
  • Loading branch information
@mstoykov
mstoykov committed Nov 10, 2023
1 parent d26bde8 commit 348aadd
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 119 deletions.
42 changes: 21 additions & 21 deletions js/tc39/breaking_test_errors.json
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion js/tc39/tc39_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -697,7 +697,7 @@ func (ctx *tc39TestCtx) runTC39Module(name, src string, includes []string, vm *g
return fs.ReadFile(currentFS, specifier.Path[1:])
},
comp)
u := &url.URL{Path: path.Join(ctx.base, name)}
u := &url.URL{Scheme: "file", Path: path.Join(ctx.base, name)}

base := u.JoinPath("..")
ms := modules.NewModuleSystem(mr, moduleRuntime.VU)
Expand Down
16 changes: 5 additions & 11 deletions lib/old_archive_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ func TestOldArchive(t *testing.T) {
testCases := map[string]string{
// map of filename to data for each main file tested
"github.com/k6io/k6/samples/example.js": `github file`,
"cdnjs.com/packages/Faker": `faker file`,
"cdnjs.com/libraries/Faker": `faker file`,
"C:/something/path2": `windows script`,
"/absolulte/path2": `unix script`,
}
Expand All @@ -75,14 +75,14 @@ func TestOldArchive(t *testing.T) {
fs := makeMemMapFs(t, map[string][]byte{
// files
"/files/github.com/k6io/k6/samples/example.js": []byte(`github file`),
"/files/cdnjs.com/packages/Faker": []byte(`faker file`),
"/files/cdnjs.com/libraries/Faker": []byte(`faker file`),
"/files/example.com/path/to.js": []byte(`example.com file`),
"/files/_/C/something/path": []byte(`windows file`),
"/files/_/absolulte/path": []byte(`unix file`),

// scripts
"/scripts/github.com/k6io/k6/samples/example.js2": []byte(`github script`),
"/scripts/cdnjs.com/packages/Faker2": []byte(`faker script`),
"/scripts/cdnjs.com/libraries/Faker2": []byte(`faker script`),
"/scripts/example.com/path/too.js": []byte(`example.com script`),
"/scripts/_/C/something/path2": []byte(`windows script`),
"/scripts/_/absolulte/path2": []byte(`unix script`),
Expand All @@ -104,9 +104,9 @@ func TestOldArchive(t *testing.T) {
"/example.com/path/to.js": []byte(`example.com file`),
"/example.com/path/too.js": []byte(`example.com script`),
"/github.com/k6io/k6/samples/example.js": []byte(`github file`),
"/cdnjs.com/packages/Faker": []byte(`faker file`),
"/cdnjs.com/libraries/Faker": []byte(`faker file`),
"/github.com/k6io/k6/samples/example.js2": []byte(`github script`),
"/cdnjs.com/packages/Faker2": []byte(`faker script`),
"/cdnjs.com/libraries/Faker2": []byte(`faker script`),
}),
}

Expand Down Expand Up @@ -157,12 +157,6 @@ func TestFilenamePwdResolve(t *testing.T) {
expectedFilenameURL: &url.URL{Opaque: "cdnjs.com/libraries/Faker"},
expectedPwdURL: &url.URL{Scheme: "file", Path: "/home/nobody"},
},
{
Filename: "example.com/something/dot.js",
Pwd: "example.com/something/",
expectedFilenameURL: &url.URL{Host: "example.com", Scheme: "", Path: "/something/dot.js"},
expectedPwdURL: &url.URL{Host: "example.com", Scheme: "", Path: "/something"},
},
{
Filename: "https://example.com/something/dot.js",
Pwd: "https://example.com/something",
Expand Down
102 changes: 39 additions & 63 deletions loader/loader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ var (
{"cdnjs", cdnjs, regexp.MustCompile(`^cdnjs\.com/libraries/([^/]+)(?:/([(\d\.)]+-?[^/]*))?(?:/(.*))?$`)},
{"github", github, regexp.MustCompile(`^github\.com/([^/]+)/([^/]+)/(.*)$`)},
}
errNoLoaderMatched = errors.New("no loader matched")
)

const (
httpsSchemeCouldntBeLoadedMsg = `The moduleSpecifier "%s" couldn't be retrieved from` +
` the resolved url "%s". Error : "%s"`
fileSchemeCouldntBeLoadedMsg = `The moduleSpecifier "%s" couldn't be found on ` +
Expand All @@ -46,30 +50,13 @@ var (
`your script and modules so that they're accessible by k6 from ` +
`inside of the container, see ` +
`https://k6.io/docs/using-k6/modules#using-local-modules-with-docker.`
nothingWorkedLoadedMsg = fileSchemeCouldntBeLoadedMsg +
` Additionally it was tried to be loaded as remote module by prepending "https://" to it, ` +
`which also didn't work. Remote resolution error: "%s"`
errNoLoaderMatched = errors.New("no loader matched")
)

// noSchemeRemoteModuleResolutionError is returned when a url with no scheme was tried to be
// resolved and errored out
type noSchemeRemoteModuleResolutionError struct {
err error // original error
moduleSpecifier string
}
type unresolvableURLError string

func (n noSchemeRemoteModuleResolutionError) Error() string {
return fmt.Sprintf(
`Module specifier "%s" was tried to be loaded as remote module by prepending "https://" to it, `+
`which didn't work. If you are trying to import a nodejs module, this is not supported `+
`as k6 is _not_ nodejs based. Please read https://k6.io/docs/using-k6/modules for more information. `+
`Remote resolution error: "%s"`, n.moduleSpecifier, n.err)
}

// Unwrap returns the wrapped error.
func (n noSchemeRemoteModuleResolutionError) Unwrap() error {
return n.err
func (u unresolvableURLError) Error() string {
// TODO potentially add more things about what k6 supports if users report being confused.
return fmt.Sprintf(`The moduleSpecifier %q couldn't be recognised as something k6 supports.`, (string)(u))
}

// Resolve a relative path to an absolute one.
Expand Down Expand Up @@ -97,18 +84,14 @@ func Resolve(pwd *url.URL, moduleSpecifier string) (*url.URL, error) {
}
return u, err
}
// here we only care if a loader is pickable, if it is and later there is an error in the loading
// from it we don't want to try another resolve
_, loader, _ := pickLoader(moduleSpecifier)
if loader == nil {
u, err := url.Parse("https://" + moduleSpecifier)
if err != nil {
return nil, noSchemeRemoteModuleResolutionError{err: err, moduleSpecifier: moduleSpecifier}
}
u.Scheme = ""
return u, nil
if loader != nil {
// here we only care if a loader is pickable, if it is and later there is an error in the loading
// from it we don't want to try another resolve
return &url.URL{Opaque: moduleSpecifier}, nil
}
return &url.URL{Opaque: moduleSpecifier}, nil

return nil, unresolvableURLError(moduleSpecifier)
}

func resolveFilePath(pwd *url.URL, moduleSpecifier string) (*url.URL, error) {
Expand Down Expand Up @@ -172,6 +155,10 @@ func Load(
}
scheme := moduleSpecifier.Scheme
if scheme == "" {
if moduleSpecifier.Opaque == "" {
//nolint:stylecheck
return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, originalModuleSpecifier)
}
scheme = "https"
}

Expand All @@ -188,43 +175,32 @@ func Load(
if !errors.Is(err, fs.ErrNotExist) {
return nil, err
}
if scheme == "https" {
finalModuleSpecifierURL := &url.URL{}

switch {
case moduleSpecifier.Opaque != "": // This is loader
finalModuleSpecifierURL, err = resolveUsingLoaders(logger, moduleSpecifier.Opaque)
if err != nil {
return nil, err
}
case moduleSpecifier.Scheme == "":
logger.Warningf(`The moduleSpecifier "%s" has no scheme but we will try to resolve it as remote module. `+
`This is deprecated and will be removed in v0.48.0 - all remote modules will `+
`need to explicitly be prepended with "https://".`, originalModuleSpecifier)
*finalModuleSpecifierURL = *moduleSpecifier
finalModuleSpecifierURL.Scheme = scheme
default:
finalModuleSpecifierURL = moduleSpecifier
}
var result *SourceData
result, err = loadRemoteURL(logger, finalModuleSpecifierURL)
if err == nil {
result.URL = moduleSpecifier
// TODO maybe make an fsext.Fs which makes request directly and than use CacheOnReadFs
// on top of as with the `file` scheme fs
_ = fsext.WriteFile(filesystems[scheme], pathOnFs, result.Data, 0o644)
return result, nil
}
if scheme != "https" {
//nolint:stylecheck
return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, originalModuleSpecifier)
}

finalModuleSpecifierURL := moduleSpecifier

if moduleSpecifier.Scheme == "" && moduleSpecifier.Opaque == "" {
// we have an error and we did remote module resolution without a scheme
// let's write the coolest error message to try to help the lost soul who got to here
return nil, noSchemeRemoteModuleResolutionError{err: err, moduleSpecifier: originalModuleSpecifier}
if moduleSpecifier.Opaque != "" { // This is a loader
finalModuleSpecifierURL, err = resolveUsingLoaders(logger, moduleSpecifier.Opaque)
if err != nil {
return nil, err
}
}

var result *SourceData
result, err = loadRemoteURL(logger, finalModuleSpecifierURL)
if err != nil {
//nolint:stylecheck
return nil, fmt.Errorf(httpsSchemeCouldntBeLoadedMsg, originalModuleSpecifier, finalModuleSpecifierURL, err)
}
result.URL = moduleSpecifier
// TODO maybe make an fsext.Fs which makes request directly and than use CacheOnReadFs
// on top of as with the `file` scheme fs
_ = fsext.WriteFile(filesystems[scheme], pathOnFs, result.Data, 0o644)

return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, originalModuleSpecifier)
return result, nil
}

func resolveUsingLoaders(logger logrus.FieldLogger, name string) (*url.URL, error) {
Expand Down
14 changes: 3 additions & 11 deletions loader/loader_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,8 @@ func TestResolve(t *testing.T) {
t.Run("Missing", func(t *testing.T) {
t.Parallel()
u, err := loader.Resolve(root, "example.com/html")
require.NoError(t, err)
assert.Equal(t, u.String(), "//example.com/html")
// TODO: check that warning will be emitted if Loaded
require.ErrorContains(t, err, "The moduleSpecifier \"example.com/html\" couldn't be recognised as something k6 supports")
require.Nil(t, u)
})
t.Run("WS", func(t *testing.T) {
t.Parallel()
Expand Down Expand Up @@ -245,18 +244,11 @@ func TestLoad(t *testing.T) {
root, err := url.Parse("file:///")
require.NoError(t, err)

t.Run("IP URL", func(t *testing.T) {
t.Parallel()
_, err := loader.Resolve(root, "192.168.0.%31")
require.Error(t, err)
require.Contains(t, err.Error(), `invalid URL escape "%31"`)
})

testData := [...]struct {
name, moduleSpecifier string
}{
{"URL", sr("HTTPSBIN_URL/invalid")},
{"HOST", "some-path-that-doesnt-exist.js"},
{"HOST", "https://some-path-that-doesnt-exist.js"},
}

filesystems := map[string]fsext.Fs{"https": fsext.NewMemMapFs()}
Expand Down
17 changes: 5 additions & 12 deletions loader/readsource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,19 +46,12 @@ func ReadSource(
pwdURL := &url.URL{Scheme: "file", Path: filepath.ToSlash(filepath.Clean(pwd)) + "/"}
srcURL, err := Resolve(pwdURL, filepath.ToSlash(src))
if err != nil {
var noSchemeError noSchemeRemoteModuleResolutionError
if errors.As(err, &noSchemeError) {
// TODO maybe try to wrap the original error here as well, without butchering the message
return nil, fmt.Errorf(nothingWorkedLoadedMsg, noSchemeError.moduleSpecifier, noSchemeError.err)
var unresolvedError unresolvableURLError
if errors.As(err, &unresolvedError) {
//nolint:stylecheck
return nil, fmt.Errorf(fileSchemeCouldntBeLoadedMsg, (string)(unresolvedError))
}
return nil, err
}
result, err := Load(logger, filesystems, srcURL, src)
var noSchemeError noSchemeRemoteModuleResolutionError
if errors.As(err, &noSchemeError) {
// TODO maybe try to wrap the original error here as well, without butchering the message
return nil, fmt.Errorf(nothingWorkedLoadedMsg, noSchemeError.moduleSpecifier, noSchemeError.err)
}

return result, err
return Load(logger, filesystems, srcURL, src)
}

0 comments on commit 348aadd

Please sign in to comment.