[fs] Add a fs.readAll method to the fs module (3/3)

[oleiade]

What?

This Pull Request adds a fs.readAll(file) operation to the filesystem module. It follows a similar behavior to Deno's, and to Go's io.ReadAll, and allows users to retrieve the whole content of a file in a single function call.

Just like with the file.read operation and the Go Reader, the operation moves the offset of the underlying file offset accordingly.

Calling readAll from an offset > 0 is supported and will read from the current offset until the end of the file.
Calling readAll from the end of the file returns null.

Why?

This operation allows to reproduce the behavior of the current open function in the fs module. It also acts as a convenience to read the whole content of a file, without having to resort to manual buffering + file.read.

It, however, differs a bit from the initial #3229 proposed design. We discussed it internally and reached the conclusion that the currently proposed design felt more idiomatic and convenient than having the operation on the file instance itself.

Checklist

• I have performed a self-review of my code.
• I have added tests for my changes.
• I have run linter locally (make lint) and all checks pass.
• I have run tests locally (make tests) and all tests pass.
• I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

ref #3229
ref #2974

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (experimental/fs-readseek@0a4f80b). Click here to learn what that means.

❗ Current head 2c8c6fd differs from pull request most recent head 91c3c0d. Consider uploading reports for the commit 91c3c0d to get more accurate results

Additional details and impacted files

@@                     Coverage Diff                     @@
##             experimental/fs-readseek    #3422   +/-   ##
===========================================================
  Coverage                            ?   73.14%           
===========================================================
  Files                               ?      263           
  Lines                               ?    20120           
  Branches                            ?        0           
===========================================================
  Hits                                ?    14717           
  Misses                              ?     4460           
  Partials                            ?      943           

Flag	Coverage Δ
ubuntu	73.07% <0.00%> (?)
windows	73.00% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codebien]

@oleiade Deno has deprecated the readAll in favor of ReadableStreams. Considering that part of the ecosystem already tracked it as a not optimal way and we want to introduce ReadableStreams as well, should we have some sort of duplicate?

[oleiade]

@codebien neither ReadableStream nor the associated ReadableStreamBYOBReader (including the broader Reader abstraction for streams) supports consuming an entire stream in a single call. When we implement streams, we can consider adding a similar readable property.

I'm currently still unclear on how easy/intuitive it will be to consume a whole Stream once we implement that specification, and would need to do a bit of tinkering to figure it out.

We had defined the readAll operation (although in the design doc as a method of the file) as a convenience to make it straightforward to obtain the whole content of a file without having to read it chunk by chunk manually.

I have no strong preference here. The alternatives to move forward at this point are:

1. Introduce this functionality still, although we'll later have streams. I slightly prefer this option as it makes users of the module's lives easier for that specific scenario.
2. Wait for our implementation of Streams before considering introducing this API. I'd also be fine with that.

Could you let me know what you prefer?

[codebien]

I'm currently still unclear on how easy/intuitive it will be to consume a whole Stream once we implement that specification, and would need to do a bit of tinkering to figure it out.

At this point, considering all the uncertainty around Streams, let's add this helper function that improves the ergonomy for developers. We may drop or deprecate it in the future if a better alternative will appear.

[mstoykov]

I don't see why at this point we will want to add ReadAll.

• if someone wants to just read the file - they can use open. Using ReadAll and this module will just impose an additioanl copy.
• this adds even more code and API to a module that is experimental.
• arguably makes the antipattern of reading the whole file easier.

I am okay with reevaluating this at some future point when the module has been around for a while, has matured and there is a need for this.

But at this point this seems to just add more code that we will need to maintain while we experiment with the module.

[oleiade]

@mstoykov 👍🏻

My main goal with readAll was to ensure that a user could use a single API/module if they chose to go with k6/experimental/fs. But I do hear the argument about it being redundant and probably premature though.

Closing this as a result 🙇🏻