fs: fix confusing flags TypeError msg

[sam-github]

Fix #2871, replace #2873. I did this on the plane back from NodeConf, it is basically the same change as #2873, but with more tests.

File open flags must be an int when passed to the binding layer, but
they must be a string when passed to the fs module (numbers are
permitted, though undocumented). The module used to do no type checking,
so the binding layer error would be thrown, and it was wrong:

> fs.openSync('_')
TypeError: flags must be an int
    at TypeError (native)
    at Object.fs.openSync (fs.js:549:18)

It is now:

> fs.openSync('_')
TypeError: flag must be a string

[bnoordhuis]

Suggestion: drop the braces for consistency or add them above.

[bnoordhuis]

LGTM

[sam-github]

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

Braces dropped.

[ronkorving]

So why not just add the tests (or ask me for them)? Personally, I think the error you now throw is incorrect (ints are fine, not just strings), and the check is incorrect too (1.5 is not a valid flag, but is a number).

[reqshark]

@ronkorving yea but the number returns flag before the string check. this looks good to me

[reqshark]

oh wait now i see what you're saying nevermind it doesn't look that good, you're right the check is wrong

[reqshark]

(numbers are permitted, though undocumented).

this whole thing would be a lot simpler if number weren't permitted. why are numbers OK at the JS layer?

[ronkorving]

I really like those numerical constants though. I would love to see them exposed and documented.

[sam-github]

@ronkorving feel free to grab bits of this and merge it into your PR

So why not just add the tests (or ask me for them)?

This code pre-existed your PR, and I think it addresses most of the comments on yours. But feel free to keep going on yours.

Personally, I think the error you now throw is incorrect (ints are fine, not just strings), and the check is incorrect too (1.5 is not a valid flag, but is a number).

Run the code. If you pass 1.5, an error is thrown (its caught at the c++ level, and the error thrown is correct in this case).

[reqshark]

@sam-github good call. I'm seeing the same

[ronkorving]

@sam-github thanks for the explanation. So we both fixed the same issue (which wasn't reported yet) in parallel? Neato :) Anyway, your solution kinda works for me, thanks for explaining.

However, if we really don't want people to pass integers, why allow it at all? Your number type check works, but still, the native TypeError will ask explicitly for integers, while that's still not what we want to convey to users... So should we allow numbers at all?

I like those constants, I have to admit, but they're not exposed nor documented and perhaps it's better that way. So perhaps just throw a TypeError if a non-string (even numbers) is passed, period?

In any case, I'll close my PR.

[jasnell]

See also: #5590

[jasnell]

Closing because #5590 landed.

[Knighton910]

👍