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: refactor "options" processing as a function #7165

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@thefourtheye
Contributor

thefourtheye commented Jun 5, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

fs

Description of change

As it is the "options" processing is repeated in all the functions
which need it. That introduces checks which are inconsistent with
similar functions and produces slightly different error messages.

This patch moves the basic "options" validation and processing to a
separate function.


cc @nodejs/fs @nodejs/collaborators

Marking this as major, as this might break some code in the wild.

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jun 5, 2016

@mscdex

View changes

lib/fs.js Outdated
@@ -41,6 +41,25 @@ const isWindows = process.platform === 'win32';
const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
const errnoException = util._errnoException;
function getOptions(options, defaultOptions) {
let actualType = typeof options;

This comment has been minimized.

@mscdex

mscdex Jun 5, 2016

Contributor

FWIW v8 still does not (IIRC) optimize storing typeof foo in a variable. It's faster to just inline the typeof without using a variable (e.g. typeof actualType === 'function').

This comment has been minimized.

@ronkorving

ronkorving Jun 6, 2016

Contributor

That's how I understand it too.

This comment has been minimized.

@thefourtheye

thefourtheye Jun 6, 2016

Contributor

Done!

@benjamingr

View changes

lib/fs.js Outdated
defaultOptions.encoding = options;
options = defaultOptions;
} else if (actualType !== 'object') {
throw new TypeError('"options" must be a string or an object');

This comment has been minimized.

@benjamingr

benjamingr Jun 5, 2016

Member

Can we add (back) the type you passed to the error message?

`"options" must be a string or an object, got ${actualType} instead.`  

This comment has been minimized.

@thefourtheye

thefourtheye Jun 6, 2016

Contributor

Done!

@benjamingr

This comment has been minimized.

Member

benjamingr commented Jun 5, 2016

Generally +1 and LGTM, left a nit.

I think this can safely be a minor - this should only break code that parses error messages - right?

@mscdex

This comment has been minimized.

Contributor

mscdex commented Jun 5, 2016

AFAIK changing error message(s) still makes it a semver-major change.

@seishun

This comment has been minimized.

Member

seishun commented Jun 6, 2016

While we're at it, either the error should say "object that isn't a function", or functions should be accepted as option bags too.

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jun 6, 2016

@seishun The default options is used because, the functions which call getOptions handle the "options passed which are actually functions" case.

@trevnorris

View changes

lib/fs.js Outdated
typeof options + ' instead.');
}
options.encoding === 'buffer' || assertEncoding(options.encoding);

This comment has been minimized.

@trevnorris

trevnorris Jun 13, 2016

Contributor

Can we please place this in an if (). no sense abusing JS's logical operators to run code.

This comment has been minimized.

@thefourtheye

thefourtheye Jun 15, 2016

Contributor

Ya sure... I changed it to an if block.

@thefourtheye thefourtheye force-pushed the thefourtheye:refactor-fs-options branch Jun 15, 2016

@silverwind

View changes

test/parallel/test-fs-read-stream-throw-type-error.js Outdated
assert.throws(function() {
fs.createReadStream(example, 0);
}, /"options" argument must be a string or an object/);
}, /"options" must be a string or an object/);

This comment has been minimized.

@silverwind

silverwind Jun 15, 2016

Contributor

Why not keep the other tests and adapt the string?

This comment has been minimized.

@thefourtheye

thefourtheye Jun 25, 2016

Contributor

@silverwind Now, I just changed the strings and allowed only null.

@silverwind

This comment has been minimized.

Contributor

silverwind commented Jun 15, 2016

LGTM with question.

@thefourtheye thefourtheye force-pushed the thefourtheye:refactor-fs-options branch 2 times, most recently Jun 25, 2016

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jun 30, 2016

Bump!

@trevnorris

View changes

lib/fs.js Outdated
@@ -41,6 +41,24 @@ const isWindows = process.platform === 'win32';
const DEBUG = process.env.NODE_DEBUG && /fs/.test(process.env.NODE_DEBUG);
const errnoException = util._errnoException;
function getOptions(options, defaultOptions) {
if (options == null || typeof options === 'function') {

This comment has been minimized.

@trevnorris

trevnorris Jun 30, 2016

Contributor

Just want to double check that using == null is intentional?

This comment has been minimized.

@thefourtheye

thefourtheye Jul 8, 2016

Contributor

Yes, @trevnorris. It is intentional.

This comment has been minimized.

@thefourtheye

thefourtheye Jul 11, 2016

Contributor

Sometimes people pass null and sometimes undefined. That is why I wrote it like this. Eg: https://github.com/isaacs/node-graceful-fs/blob/master/graceful-fs.js#L89

This comment has been minimized.

@ronkorving

ronkorving Aug 19, 2016

Contributor

(a bit late, sorry)
My only beef with this is that next time a developer sees this, they may become eager to "fix" this, causing either a bug or a new discussion about this pattern. My preference is to just be explicit and check for null and undefined explicitly with ===. Just my 2 cents.

This comment has been minimized.

@thefourtheye

thefourtheye Aug 19, 2016

Contributor

@ronkorving Fair Enough. Changed it.

@trevnorris

This comment has been minimized.

Contributor

trevnorris commented Jun 30, 2016

One question, otherwise LGTM

@thefourtheye thefourtheye force-pushed the thefourtheye:refactor-fs-options branch Jul 11, 2016

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jul 12, 2016

Okay, added a commit to fix #7655 as well. PTAL.

@Fishrock123

View changes

lib/fs.js Outdated
if (options.encoding !== 'buffer')
assertEncoding(options.encoding);
return Object.assign(defaultOptions, options);

This comment has been minimized.

@Fishrock123

Fishrock123 Jul 12, 2016

Member

We should probably only do this where we need to modify it, otherwise there will be a hit to each call

This comment has been minimized.

@thefourtheye

thefourtheye Jul 12, 2016

Contributor

@Fishrock123 You mean if we don't actually modify options, we don't want to create this copy?

This comment has been minimized.

@Fishrock123

This comment has been minimized.

@thefourtheye

thefourtheye Jul 12, 2016

Contributor

Hmmm, that's what I am afraid of. Then the actual logic of options processing will not at the same place. Perhaps we can have a second parameter in the function which can be used to flag if a copy has to be made. What do you think?

This comment has been minimized.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

@Fishrock123 I changed it to util._extend from Object.assign. Hope the hit will not be that much now, at least as per #7655 (comment)

This comment has been minimized.

@ronkorving

ronkorving Jul 13, 2016

Contributor

How does util._extend solve this problem? By the way, Object.assign should've become quite a bit faster since V8 5.1 (see http://v8project.blogspot.jp/2016/04/v8-release-51.html). Now I do believe someone was adding a benchmark to compare the two, so I'm not making the assertion that util._extends should be avoided (yet). But a copy is a copy, and is probably more than we need in quite a few cases.

This comment has been minimized.

@thefourtheye

thefourtheye Jul 13, 2016

Contributor

To think of it, it just copies things to defaultOptions. That object is constructed already in memory. Would this still be a problem?

@micnic

This comment has been minimized.

Contributor

micnic commented Jul 13, 2016

LGTM
I'm just wondering if we should also fix #7655 for v4.x or not

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jul 13, 2016

@micnic how do you think it could be done in a non semver major way?

@micnic

This comment has been minimized.

Contributor

micnic commented Jul 15, 2016

@thealphanerd something like options = util._extend({}, options); as the first expression in the affected methods would do the job, I don't think it is a semver major because it's an expected behavior and everywhere in the core modules except for some fs methods the external objects are not modified. In any case the impact of this change would be minimal, I could discover this only by using Object.freeze(), most of the users are using inline defined objects without reusing or freezing them.

@thefourtheye thefourtheye force-pushed the thefourtheye:refactor-fs-options branch Jul 21, 2016

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jul 21, 2016

@thefourtheye thefourtheye force-pushed the thefourtheye:refactor-fs-options branch 3 times, most recently Jul 22, 2016

@addaleax

This comment has been minimized.

Member

addaleax commented Aug 27, 2016

@thealphanerd started a comparison CITGM run with v6.3.1 yesterday with the same vinyl-fs failures: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/371/ (from #8253 (comment)).

That shows the same failures, so the failures here are likely unrelated…

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Aug 27, 2016

/cc @phated

On Sat, Aug 27, 2016, 11:02 AM Anna Henningsen notifications@github.com
wrote:

@thealphanerd https://github.com/TheAlphaNerd started a comparison
CITGM run with v6.3.1 yesterday with the same vinyl-fs failures:
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/371/ (from #8253
(comment)
#8253 (comment)).

That shows the same failures, so the failures here are likely unrelated…


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7165 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecVyWwn5yzmRmF-aUFNFUWrqXpBP_iks5qkBlSgaJpZM4IudDT
.

@thefourtheye thefourtheye force-pushed the thefourtheye:refactor-fs-options branch Sep 2, 2016

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Sep 2, 2016

Squashed and Rebased.

@Trott Trott force-pushed the nodejs:master branch to c5ce7f4 Sep 21, 2016

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Sep 28, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Sep 30, 2016

CI looks good other than a known flaky failure. @thefourtheye, do you want to go ahead and get this landed?

fs: refactor "options" processing as a function
As it is, the "options" processing is repeated in all the functions
which need it. That introduces checks which are inconsistent with
other functions and produces slightly different error messages.

This patch moves the basic "options" validation and processing to a
seperate function.

@thefourtheye thefourtheye force-pushed the thefourtheye:refactor-fs-options branch to 4693ee5 Oct 5, 2016

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Oct 5, 2016

Landed in e8e969a. Thanks for the follow-up @jasnell :-)

thefourtheye added a commit that referenced this pull request Oct 5, 2016

fs: refactor "options" processing as a function
As it is, the "options" processing is repeated in all the functions
which need it. That introduces checks which are inconsistent with
other functions and produces slightly different error messages.

This patch moves the basic "options" validation and processing to a
seperate function.

PR-URL: #7165
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@thefourtheye thefourtheye deleted the thefourtheye:refactor-fs-options branch Oct 5, 2016

@addaleax

This comment has been minimized.

Member

addaleax commented Oct 5, 2016

Is this only semver-major due to the changed error messages? If so, would it make sense to pull this into v7.0.0? To me the diff looks like it could likely cause conflicts when backporting to v7 otherwise…

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Oct 5, 2016

@addaleax The encoding assertion happens for all the functions now. That also would qualify as a major change I believe.

@addaleax

This comment has been minimized.

Member

addaleax commented Oct 5, 2016

Okay then, that’s a bit more than just changed messages. I’m not sure, but maybe it’s still worth to ping @nodejs/ctc and see if anybody feels strongly about it?

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 5, 2016

yeah, prefer major.

If you want it in v7 we should discuss this now

@jasnell

This comment has been minimized.

Member

jasnell commented Oct 6, 2016

I'm ok with pulling this into v7. @nodejs/ctc ... thoughts?

@addaleax

This comment has been minimized.

Member

addaleax commented Oct 6, 2016

Seems okay to me, yes. 👍

@jasnell

This comment has been minimized.

Member

jasnell commented Oct 6, 2016

If there are no objections from @nodejs/ctc by Monday, I'll pull this in to v7.x-staging

@jasnell jasnell added this to the 7.0.0 milestone Oct 6, 2016

@jasnell jasnell referenced this pull request Oct 10, 2016

Closed

fs: don't alter user provided `options` object #7831

3 of 3 tasks complete

jasnell added a commit that referenced this pull request Oct 10, 2016

fs: refactor "options" processing as a function
As it is, the "options" processing is repeated in all the functions
which need it. That introduces checks which are inconsistent with
other functions and produces slightly different error messages.

This patch moves the basic "options" validation and processing to a
seperate function.

PR-URL: #7165
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

kevinoid added a commit to kevinoid/fs-file-sync-fd that referenced this pull request Dec 13, 2016

fs: refactor "options" processing as a function
As it is, the "options" processing is repeated in all the functions
which need it. That introduces checks which are inconsistent with
other functions and produces slightly different error messages.

This patch moves the basic "options" validation and processing to a
seperate function.

PR-URL: nodejs/node#7165
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

kevinoid added a commit to kevinoid/fs-file-sync-fd that referenced this pull request Dec 13, 2016

fs: refactor "options" processing as a function
As it is, the "options" processing is repeated in all the functions
which need it. That introduces checks which are inconsistent with
other functions and produces slightly different error messages.

This patch moves the basic "options" validation and processing to a
seperate function.

PR-URL: nodejs/node#7165
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@thefourtheye thefourtheye referenced this pull request Mar 16, 2017

Closed

fs: getOption to populate passed in default values #11630

2 of 2 tasks complete

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment