Skip to content

Commit

Permalink
Fix all linter errors in loader package
Browse files Browse the repository at this point in the history
This removes 67 linter errors, does not (seem) to introduce any races in
the tests and it also seems to not improve their runtime as well :(

The changes were done with the idea of "as little change as possible" so
that there should be no bugs introduced.
  • Loading branch information
mstoykov committed Jan 25, 2022
1 parent 1ed7af9 commit e9a6d8f
Show file tree
Hide file tree
Showing 5 changed files with 141 additions and 82 deletions.
6 changes: 5 additions & 1 deletion loader/cdnjs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (

func TestCDNJS(t *testing.T) {
t.Skip("skipped to avoid inconsistent API responses")
t.Parallel()

paths := map[string]struct {
parts []string
Expand Down Expand Up @@ -69,12 +70,13 @@ func TestCDNJS(t *testing.T) {
},
}

var root = &url.URL{Scheme: "https", Host: "example.com", Path: "/something/"}
root := &url.URL{Scheme: "https", Host: "example.com", Path: "/something/"}
logger := logrus.New()
logger.SetOutput(testutils.NewTestOutput(t))
for path, expected := range paths {
path, expected := path, expected
t.Run(path, func(t *testing.T) {
t.Parallel()
name, loader, parts := pickLoader(path)
assert.Equal(t, "cdnjs", name)
assert.Equal(t, expected.parts, parts)
Expand All @@ -96,6 +98,7 @@ func TestCDNJS(t *testing.T) {
}

t.Run("cdnjs.com/libraries/nonexistent", func(t *testing.T) {
t.Parallel()
path := "cdnjs.com/libraries/nonexistent"
name, loader, parts := pickLoader(path)
assert.Equal(t, "cdnjs", name)
Expand All @@ -105,6 +108,7 @@ func TestCDNJS(t *testing.T) {
})

t.Run("cdnjs.com/libraries/Faker/3.1.0/nonexistent.js", func(t *testing.T) {
t.Parallel()
path := "cdnjs.com/libraries/Faker/3.1.0/nonexistent.js"
name, loader, parts := pickLoader(path)
assert.Equal(t, "cdnjs", name)
Expand Down
11 changes: 8 additions & 3 deletions loader/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import (
)

func TestGithub(t *testing.T) {
t.Parallel()
logger := logrus.New()
logger.SetOutput(testutils.NewTestOutput(t))
path := "github.com/github/gitignore/Go.gitignore"
Expand All @@ -44,12 +45,13 @@ func TestGithub(t *testing.T) {
assert.NoError(t, err)
assert.Equal(t, expectedEndSrc, src)

var root = &url.URL{Scheme: "https", Host: "example.com", Path: "/something/"}
root := &url.URL{Scheme: "https", Host: "example.com", Path: "/something/"}
resolvedURL, err := Resolve(root, path)
require.NoError(t, err)
require.Empty(t, resolvedURL.Scheme)
require.Equal(t, path, resolvedURL.Opaque)
t.Run("not cached", func(t *testing.T) {
t.Parallel()
data, err := Load(logger, map[string]afero.Fs{"https": afero.NewMemMapFs()}, resolvedURL, path)
require.NoError(t, err)
assert.Equal(t, data.URL, resolvedURL)
Expand All @@ -58,10 +60,11 @@ func TestGithub(t *testing.T) {
})

t.Run("cached", func(t *testing.T) {
t.Parallel()
fs := afero.NewMemMapFs()
testData := []byte("test data")

err := afero.WriteFile(fs, "/github.com/github/gitignore/Go.gitignore", testData, 0644)
err := afero.WriteFile(fs, "/github.com/github/gitignore/Go.gitignore", testData, 0o644)
require.NoError(t, err)

data, err := Load(logger, map[string]afero.Fs{"https": fs}, resolvedURL, path)
Expand All @@ -71,7 +74,8 @@ func TestGithub(t *testing.T) {
})

t.Run("relative", func(t *testing.T) {
var tests = map[string]string{
t.Parallel()
tests := map[string]string{
"./something.else": "github.com/github/gitignore/something.else",
"../something.else": "github.com/github/something.else",
"/something.else": "github.com/something.else",
Expand All @@ -84,6 +88,7 @@ func TestGithub(t *testing.T) {
})

t.Run("dir", func(t *testing.T) {
t.Parallel()
require.Equal(t, &url.URL{Opaque: "github.com/github/gitignore"}, Dir(resolvedURL))
})
}
74 changes: 43 additions & 31 deletions loader/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,11 @@
*
*/

// Package loader is about loading files from either the filesystem or through https requests.
package loader

import (
"context"
"errors"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -97,33 +99,7 @@ func Resolve(pwd *url.URL, moduleSpecifier string) (*url.URL, error) {
}

if moduleSpecifier[0] == '.' || moduleSpecifier[0] == '/' || filepath.IsAbs(moduleSpecifier) {
if pwd.Opaque != "" { // this is a loader reference
parts := strings.SplitN(pwd.Opaque, "/", 2)
if moduleSpecifier[0] == '/' {
return &url.URL{Opaque: path.Join(parts[0], moduleSpecifier)}, nil
}
return &url.URL{Opaque: path.Join(parts[0], path.Join(path.Dir(parts[1]+"/"), moduleSpecifier))}, nil
}

// The file is in format like C:/something/path.js. But this will be decoded as scheme `C`
// ... which is not what we want we want it to be decode as file:///C:/something/path.js
if filepath.VolumeName(moduleSpecifier) != "" {
moduleSpecifier = "/" + moduleSpecifier
}

// we always want for the pwd to end in a slash, but filepath/path.Clean strips it so we read
// it if it's missing
var finalPwd = pwd
if pwd.Opaque != "" {
if !strings.HasSuffix(pwd.Opaque, "/") {
finalPwd = &url.URL{Opaque: pwd.Opaque + "/"}
}
} else if !strings.HasSuffix(pwd.Path, "/") {
finalPwd = &url.URL{}
*finalPwd = *pwd
finalPwd.Path += "/"
}
return finalPwd.Parse(moduleSpecifier)
return resolveFilePath(pwd, moduleSpecifier)
}

if strings.Contains(moduleSpecifier, "://") {
Expand Down Expand Up @@ -155,6 +131,36 @@ func Resolve(pwd *url.URL, moduleSpecifier string) (*url.URL, error) {
return &url.URL{Opaque: moduleSpecifier}, nil
}

func resolveFilePath(pwd *url.URL, moduleSpecifier string) (*url.URL, error) {
if pwd.Opaque != "" { // this is a loader reference
parts := strings.SplitN(pwd.Opaque, "/", 2)
if moduleSpecifier[0] == '/' {
return &url.URL{Opaque: path.Join(parts[0], moduleSpecifier)}, nil
}
return &url.URL{Opaque: path.Join(parts[0], path.Join(path.Dir(parts[1]+"/"), moduleSpecifier))}, nil
}

// The file is in format like C:/something/path.js. But this will be decoded as scheme `C`
// ... which is not what we want we want it to be decode as file:///C:/something/path.js
if filepath.VolumeName(moduleSpecifier) != "" {
moduleSpecifier = "/" + moduleSpecifier
}

// we always want for the pwd to end in a slash, but filepath/path.Clean strips it so we read
// it if it's missing
finalPwd := pwd
if pwd.Opaque != "" {
if !strings.HasSuffix(pwd.Opaque, "/") {
finalPwd = &url.URL{Opaque: pwd.Opaque + "/"}
}
} else if !strings.HasSuffix(pwd.Path, "/") {
finalPwd = &url.URL{}
*finalPwd = *pwd
finalPwd.Path += "/"
}
return finalPwd.Parse(moduleSpecifier)
}

// Dir returns the directory for the path.
func Dir(old *url.URL) *url.URL {
if old.Opaque != "" { // loader
Expand Down Expand Up @@ -203,7 +209,7 @@ func Load(
return nil, err
}
if scheme == "https" {
var finalModuleSpecifierURL = &url.URL{}
finalModuleSpecifierURL := &url.URL{}

switch {
case moduleSpecifier.Opaque != "": // This is loader
Expand All @@ -226,7 +232,7 @@ func Load(
result.URL = moduleSpecifier
// TODO maybe make an afero.Fs which makes request directly and than use CacheOnReadFs
// on top of as with the `file` scheme fs
_ = afero.WriteFile(filesystems[scheme], pathOnFs, result.Data, 0644)
_ = afero.WriteFile(filesystems[scheme], pathOnFs, result.Data, 0o644)
return result, nil
}

Expand Down Expand Up @@ -255,7 +261,7 @@ func resolveUsingLoaders(logger logrus.FieldLogger, name string) (*url.URL, erro
}

func loadRemoteURL(logger logrus.FieldLogger, u *url.URL) (*SourceData, error) {
var oldQuery = u.RawQuery
oldQuery := u.RawQuery
if u.RawQuery != "" {
u.RawQuery += "&"
}
Expand Down Expand Up @@ -292,7 +298,13 @@ func pickLoader(path string) (string, loaderFunc, []string) {
func fetch(logger logrus.FieldLogger, u string) ([]byte, error) {
logger.WithField("url", u).Debug("Fetching source...")
startTime := time.Now()
res, err := http.Get(u)
ctx, cancel := context.WithTimeout(context.Background(), time.Minute)
defer cancel()
req, err := http.NewRequestWithContext(ctx, http.MethodGet, u, nil)
if err != nil {
return nil, err
}
res, err := http.DefaultClient.Do(req)
if err != nil {
return nil, err
}
Expand Down
Loading

0 comments on commit e9a6d8f

Please sign in to comment.