fs: getOption to populate passed in default values

[fhalde]

In fs.js, the getOption can be improved to populate the option object with the passed in defaultOptions if the default options are not there in the original option object. This way we don't have to do options.flag || 'w' etc.

For example in the fs.writeFile

fs.writeFile = function(file, options, cb) {
    options = getOptions(options, {flag: 'w'});
    /* later somewhere */
    flags = options.flags || 'w'
   
    /* could have just been the below had getOptions populated the default values if it did not exist */
    flags = options.flags 
}

Similar patterns are present in few more parts of the fs module

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

Affected core subsystem(s)

fs, test

[sam-github]

This PR would be easier to understand if the description contained some example code that does not work as expected before your change (with a description of what you consider expected).

[fhalde]

@sam-github There is nothing that is working unexpectedly. I just felt we are duplicating the work of checking if default values exists or not in the options.

For e.g.

fs.writeFile = function(file, options, cb) {
    options = getOptions(options, {flag: 'w'});
    /* later somewhere */
    flags = options.flags || 'w'
   
   /* could have just been the below had getOptions populated the default values if not exist */
   flags = options.flags 
}

[fhalde]

Updated the description to explain what this PR would lead to

[jasnell]

@nodejs/collaborators ... thoughts?

[jasnell]

CI: https://ci.nodejs.org/job/node-test-pull-request/6853/

[mscdex]

Have you benchmarked this? I thought Object.assign() was still slow?

[fhalde]

never done a benchmark. do we have any existing ones to compare it with? or any suggestions how to do a benchmark? Thanks

[fhalde]

I was suggested to use util.extend, would it be faster ?

[mscdex]

Yes, util._extend() would probably be a better choice, at least in core. Object.assign() is probably fine for tests and non-runtime stuff.

[thefourtheye]

Related discussion: #7165 (comment)

[rmg]

Why are the tests changed to use Object.assign instead of the original Object.create?

[fhalde]

@rmg I can recall that object.create puts the property in the prototype of the object. Due to this, object.assign would not copy those properties.

[rmg]

@fhalde

Due to this, object.assign would not copy those properties.

Doh! I should have caught that 😊

[fhalde]

@rmg it shouldn't be a problem though right ? I mean was there a reason why in the tests the properties were put in the prototype ?

[rmg]

@fhalde after some digging, it seems those tests are intentionally written to use Object.create because they are there to ensure options can be inherited. See #635 for the PR and previous discussion.

edit forgot to add, this is actually what the -inherit in the test file's name means. So basically Object.assign can't be used because it breaks the existing tests which are there specifically to guard against changes like this.

[fhalde]

@rmg
Then let me revert the test code changes. This would cause the test to fail however. Any utility function allowing us to copy prototype properties as well ?
Also as part of #7165 (comment) , it looks like what I am trying to achieve was discarded previously

[rmg]

@fhalde it seems like this might end up being a lost cause then, since a solution that supports prototypes might end up being more complicated than just leaving things as they are :-(

[fhalde]

Agree too. I'll close it ? @rmg

[rmg]

@fhalde agreed, may as well close this.

FWIW, I liked the idea behind the change.

[jasnell]

Closing based on the discussion.