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: remove unnecessary usage of .hasOwnProperty() #635

Closed
wants to merge 1 commit into from
Closed

fs: remove unnecessary usage of .hasOwnProperty() #635

wants to merge 1 commit into from

Conversation

jonathanong
Copy link
Contributor

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

AFAIK, .hasOwnProperty() is unnecessary here.

@vkurchatkin
Copy link
Contributor

maybe add a test for this to prevent regressions?

@vkurchatkin vkurchatkin added the fs Issues and PRs related to the fs subsystem / file system. label Jan 28, 2015
@domenic
Copy link
Contributor

domenic commented Jan 28, 2015

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

@jonathanong
Copy link
Contributor Author

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

@vkurchatkin what tests, specifically?

@vkurchatkin
Copy link
Contributor

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

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

domenic commented Jan 28, 2015

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

domenic commented Jan 28, 2015

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

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

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

What is different in ES6 that makes hasOwnProperty unnecessary?

@vkurchatkin
Copy link
Contributor

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

@jonathanong ping

@jonathanong
Copy link
Contributor Author

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

@vkurchatkin
Copy link
Contributor

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

options = util._extend({
highWaterMark: 64 * 1024
}, options || {});
options = Object.create(options || {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@tellnes
Copy link
Contributor

tellnes commented Feb 20, 2015

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

@vkurchatkin
Copy link
Contributor

LGTM

@vkurchatkin vkurchatkin added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 23, 2015
vkurchatkin pushed a commit that referenced this pull request Mar 5, 2015
@vkurchatkin
Copy link
Contributor

Landed in 4d0329e

@vkurchatkin vkurchatkin closed this Mar 5, 2015
@rvagg rvagg mentioned this pull request Mar 5, 2015
@jonathanong jonathanong deleted the fs-remove-hasownproperty branch March 5, 2015 17:01
rvagg added a commit that referenced this pull request Mar 6, 2015
Notable changes:

* buffer: New `Buffer#indexOf()` method, modelled off `Array#indexOf()`.
  Accepts a String, Buffer or a Number. Strings are interpreted as UTF8.
  (Trevor Norris) #561
* fs: `options` object properties in `'fs'` methods no longer perform a
  `hasOwnProperty()` check, thereby allowing options objects to have
  prototype properties that apply. (Jonathan Ong)
  #635
* tls: A likely TLS memory leak was reported by PayPal. Some of the recent
  changes in stream_wrap appear to be to blame. The initial fix is in
  #1078, you can track the progress
  toward closing the leak at
  #1075 (Fedor Indutny).
* npm: Upgrade npm to 2.7.0. See npm CHANGELOG.md:
  https://github.com/npm/npm/blob/master/CHANGELOG.md#v270-2015-02-26
  for details including why this is a semver-minor when it could have
  been semver-major.
* TC: Colin Ihrig (@cjihrig) resigned from the TC due to his desire to do
  more code and fewer meetings.
brendanashworth added a commit to brendanashworth/io.js that referenced this pull request Mar 14, 2015
This commit does various changes to cleanup and prep the 8ff6b81 for
merging, which include updating the commit to follow new codebase
guidelines. The following are the changes:

doc/api/http.markdown:
  - document options
lib/http.js:
  - no changes
lib/_http_server.js
  - changes regarding isObject (nodejs#647) and hasOwnProperty (nodejs#635)
  - take tls option rather than guessing based on options
test/simple/test-https-from-http.js:
  - moved to parallel directory, crypto test, removed copyright banner
test/parallel/test-http-server.js:
  - adjust for tls option
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. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants