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

fix: fs.readFile: make options optional #11185

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
2 participants
@felixrieseberg
Member

felixrieseberg commented Nov 20, 2017

Node documents options in fs.readFile/fs.writeFile operations as optional. We have done so for almost all operations, but made a mistake in fs.readFile - if you don't pass options, you'll get a TypeError that shouldn't be there.

This tiny PR fixes that.

Closes #11182

@felixrieseberg felixrieseberg requested a review from electron/reviewers as a code owner Nov 20, 2017

@ckerr

ckerr approved these changes Nov 20, 2017 edited

LGTM.

As an aside, I notice util.isString() and util.isObject() are deprecated. Any idea what the supported way is for doing that now?

@ckerr ckerr merged commit fa37d49 into master Nov 20, 2017

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@ckerr ckerr deleted the fs-optional-options branch Nov 20, 2017

@felixrieseberg

This comment has been minimized.

Member

felixrieseberg commented Nov 20, 2017

@ckerr Oh, good catch! Core moved those out into userland (https://github.com/isaacs/core-util-is), with the argument being that those have no place in core. I'll make another PR that goes with the module rather than the deprecated functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment