fs: remove unnecessary usage of .hasOwnProperty()

[jonathanong]

on a mission to remove all instances of util._extend() and allow options to have prototypes.

AFAIK, .hasOwnProperty() is unnecessary here.

[vkurchatkin]

maybe add a test for this to prevent regressions?

[domenic]

ES6 style would be === undefined not in. Would suggest that for easier refactoring to ES6 defaults when they become available.

[jonathanong]

okay i'll change it to @domenic's recommendations.

@vkurchatkin what tests, specifically?

[vkurchatkin]

@domenic not the same thing. what if { someOption: undefined } means that option should be undefined, not default value

@jonathanong like, that you pass Object.create(options) and this works

[jonathanong]

@domenic actually, would == null be better? an explanation would be nice :D i have a feeling some people set value options to null, and doing === would break backwards compat

[domenic]

@vkurchatkin in ES2015, undefined is specifically used to trigger defaults.

null is not used to trigger defaults. In general null is "in-alphabet" whereas undefined is the default-triggering sentinel value.

[domenic]

Concrete examples:

function f(x = 1, y = 2) {
  console.log(x, y);
}

function g({ x = 1, y = 2 }) {
  console.log(x, y);
}

f(); // 1, 2
f(undefined, undefined); // 1, 2
f(null, null); // null, null

g({ x: undefined, y: undefined }); // 1, 2
g({}); // 1, 2
g({ x: null, x: null }); // null, null

[vkurchatkin]

@domenic thanks! we should do the same, guess. But I'm afraid that in same cases swapping hasOwnProperty with === undefined would be a breaking change

[kevinmartin]

I agree with @domenic that === undefined is the way to go. It's the correct way going forward with ES6 to make use of default values. The only thing this would "potentially" break is if there is a value where the default is null but someone enters (and means for the value to specifically be) undefined.

I don't see that as a problem either way, because the only variable that it would affect is this.fd. The reason why it doesn't affect it is because every function that utilizes this.fd (except SyncWriteStream.prototype.write for some reason), does a util.isNumber() check on this.fd. Therefore it doesn't matter if it's null or undefined

[piscisaureus]

What is different in ES6 that makes hasOwnProperty unnecessary?

[vkurchatkin]

@piscisaureus it's not about ES6, I think. @jonathanong want's to get reed of hasOwnProperty so that options could be inherited. @domenic says that it's not ES6 way to use in instead. If you write something like this:

function fn({ option1, encoding='utf8'}) {
}

encoding will be utf8 for fn({ encoding:undefined })

[vkurchatkin]

@jonathanong ping

[jonathanong]

@vkurchatkin sup? Which direction should I take this PR?

[vkurchatkin]

1. Use === undefined
2. Add some tests where options are inherited

[jonathanong]

added this because i realized that without it, all my .hasOwnProperty() stuff is futile

[vkurchatkin]

I think this is fine since ReadableStream doesn't use .hasOwnProperty

[tellnes]

@jonathanong I think something has gone wrong with your rebaseing.

[vkurchatkin]

LGTM

[vkurchatkin]

Landed in 4d0329e