Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
oleiade committed Nov 7, 2023
1 parent 7d7a131 commit fa8dfe1
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 17 deletions.
12 changes: 6 additions & 6 deletions js/modules/k6/experimental/fs/cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import (
"path/filepath"
"sync"

"github.com/spf13/afero"
"go.k6.io/k6/lib/fsext"
)

// cache is a cache of opened files.
Expand Down Expand Up @@ -40,14 +40,14 @@ type cache struct {
//
// Parameters:
// - filename: The name of the file to be retrieved. This should be a relative or absolute path.
// - fromFs: The filesystem (from the afero package) from which the file should be read if not already cached.
// - fromFs: The filesystem (from the fsext package) from which the file should be read if not already cached.
//
// Returns:
// - A byte slice containing the content of the specified file.
// - An error if there's any issue opening or reading the file. If the file content is
// successfully cached and returned once, subsequent calls will not produce
// file-related errors for the same file, as the cached value will be used.
func (fr *cache) open(filename string, fromFs afero.Fs) (data []byte, err error) {
func (fr *cache) open(filename string, fromFs fsext.Fs) (data []byte, err error) {
filename = filepath.Clean(filename)

if f, ok := fr.openedFiles.Load(filename); ok {
Expand All @@ -59,9 +59,9 @@ func (fr *cache) open(filename string, fromFs afero.Fs) (data []byte, err error)
return data, nil
}

// The underlying afero.Fs.Open method will cache the file content during this
// The underlying fsext.Fs.Open method will cache the file content during this
// operation. Which will lead to effectively holding the content of the file in memory twice.
// However, as per #1079, we plan to eventually reduce our dependency on afero, and
// However, as per #1079, we plan to eventually reduce our dependency on fsext, and
// expect this issue to be resolved at that point.
// TODO: re-evaluate opening from the FS this once #1079 is resolved.
f, err := fromFs.Open(filename)
Expand All @@ -77,7 +77,7 @@ func (fr *cache) open(filename string, fromFs afero.Fs) (data []byte, err error)

data, err = io.ReadAll(f)
if err != nil {
return nil, err
return nil, fmt.Errorf("failed to read the content of file %s: %w", filename, err)
}

fr.openedFiles.Store(filename, data)
Expand Down
11 changes: 0 additions & 11 deletions js/modules/k6/experimental/fs/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,6 @@ func (f *File) Read(into goja.Value) *goja.Promise {
return promise
}

// We expect the into argument to be a `Uint8Array` instance

// intoObj := into.ToObject(f.vu.Runtime())
// uint8ArrayConstructor := f.vu.Runtime().Get("Uint8Array")
// if isUint8Array := intoObj.Get("constructor").SameAs(uint8ArrayConstructor); !isUint8Array {
intoObj := into.ToObject(f.vu.Runtime())
if !isUint8Array(f.vu.Runtime(), intoObj) {
reject(newFsError(TypeError, "read() failed; reason: into argument must be a Uint8Array"))
Expand Down Expand Up @@ -233,12 +228,6 @@ func (f *File) Read(into goja.Value) *goja.Promise {
return nil
}

// The [file.Read] method will return an EOFError as soon as it reached
// the end of the file.
//
// However, following deno's behavior, we express
// EOF to users by returning null, when and only when there aren't any
// more bytes to read.
var fsErr *fsError
isFSErr := errors.As(readErr, &fsErr)

Expand Down

0 comments on commit fa8dfe1

Please sign in to comment.