Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: getOption to populate passed in default values #11630

Closed
wants to merge 1 commit into from

Conversation

fhalde
Copy link
Contributor

@fhalde fhalde commented Mar 1, 2017

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

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.
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Mar 1, 2017
@sam-github
Copy link
Contributor

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
Copy link
Contributor Author

fhalde commented Mar 1, 2017

@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
Copy link
Contributor Author

fhalde commented Mar 1, 2017

Updated the description to explain what this PR would lead to

@jasnell
Copy link
Member

jasnell commented Mar 15, 2017

@nodejs/collaborators ... thoughts?

@jasnell
Copy link
Member

jasnell commented Mar 15, 2017

@@ -56,7 +56,7 @@ function getOptions(options, defaultOptions) {

if (options.encoding !== 'buffer')
assertEncoding(options.encoding);
return options;
return Object.assign({}, defaultOptions, options);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@mscdex mscdex Mar 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related discussion: #7165 (comment)

@rmg
Copy link
Contributor

rmg commented Mar 15, 2017

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

@fhalde
Copy link
Contributor Author

fhalde commented Mar 15, 2017

@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
Copy link
Contributor

rmg commented Mar 15, 2017

@fhalde

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

Doh! I should have caught that 😊

@fhalde
Copy link
Contributor Author

fhalde commented Mar 16, 2017

@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
Copy link
Contributor

rmg commented Mar 20, 2017

@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
Copy link
Contributor Author

fhalde commented Mar 21, 2017

@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
Copy link
Contributor

rmg commented Mar 23, 2017

@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
Copy link
Contributor Author

fhalde commented Mar 26, 2017

Agree too. I'll close it ? @rmg

@rmg
Copy link
Contributor

rmg commented Mar 27, 2017

@fhalde agreed, may as well close this.

FWIW, I liked the idea behind the change.

@jasnell
Copy link
Member

jasnell commented Mar 27, 2017

Closing based on the discussion.

@jasnell jasnell closed this Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants