fs: make read parameters optional

At least in the promisifed version the parameters after fd should be optional, i.e:

function read(fd, buffer = new Buffer(16384), offset = 0, length = buffer.length, position = null);

The non promisified is a bitter more tricky but should be doable as well.

Good first issue?

@ronag

@ronag I'd be willing to help out on this one. Just might need a little guidance to get started :)

I've played around with this a little and i think fd and buffer are needed to keep the implementation simpler, but the other values can be optional. Similar to the fs.write and fs.writev

Link to my first pass commit: lholmquist@a012cdb

In my first round of testing, i create the function signature like this:

function read(fd, buffer = Buffer.alloc(16384), offset = 0, length = buffer.length, position = null, callback)

If i run this with an example like

fs.read(fd, /*buffer, offset, length, position,*/ function cb(err, bytesRead, buffer) {
    console.log(buffer.toString('utf8'));
  })

I'll get an error because at the moment, the callback is going into the buffer value. It seems like the implementation might be simplier and less error prone if the only optional values were offset, length and position. I'm going to give that a try

You can't use default parameters here since the last parameter is not optional. Instead you might have to do something like:

read(...args) {
  const callback = args.pop();
  const [ buffer = Buffer.alloc(16384), offset = 0, length = buffer.length, position = null ] = args;
}

Will need to check the performance implications as well.

I have something working here: b139697 still using the default params(except for the callback). It is doing a little more typeof checking than i think is needed

I'll give the spread stuff a shot and see how that goes

@ronag

i'm not a performance expert, so i don't if this is an issue, but it seem like creating that an extra default buffer if we didn't need to might affect something? Maybe not making buffer optional might be a good thing? 🤷‍♂️

I'll send a PR in the next day or so for more discussion

Thanks @ronag for the help

but it seem like creating that an extra default buffer if we didn't need to might affect something

It would only be created if no buffer is provided. Default parameters are evaluated lazylily as far as I know. Might be worth to confirm though.

Default parameters are evaluated lazylily as far as I know. Might be worth to confirm though.

👍

feat: make fs.read params optional

fs: make fs.read params optional

Navigation Menu

Repository navigation

fs: make read parameters optional #31237

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

fs: make read parameters optional #31237

Description

Activity

ronag commented on Jan 7, 2020

lholmquist commented on Jan 15, 2020

lholmquist commented on Jan 16, 2020

ronag commented on Jan 16, 2020

lholmquist commented on Jan 16, 2020

lholmquist commented on Jan 16, 2020

ronag commented on Jan 16, 2020

lholmquist commented on Jan 16, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions