FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: fix readFile will pass undefined to callback but not null by zbinlin · Pull Request #3740 · nodejs/node · GitHub

/ node Public

fs: fix readFile will pass undefined to callback but not null - #3740

Closed
zbinlin wants to merge 1 commit into
nodejs:masterfrom
zbinlin:fix-bug-branch
ConversationCommits (1)ChecksFiles changed
Closed

fs: fix readFile will pass undefined to callback but not null#3740
zbinlin wants to merge 1 commit into
nodejs:masterfrom
zbinlin:fix-bug-branch

Conversation

zbinlin commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

In PR-3485, if encoding set, fs.readFile will pass undefined not null to callback.

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

I suppose this doesn't hurt for a little extra consistency. LGTM

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

Care to change the var to let?

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 10, 2015

zbinlin commented Nov 10, 2015

Copy link
Copy Markdown
Contributor Author

In my node's package promise-adapter, I am not sure a callback whether is node-style or not，so I suppose if the first arguments is null that is node-style callback.

Other cases, I think this has little effect.

evanlucas commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

I'm not sure how reliable that will be. I'd be willing to bet that there are cases where the first arg to the callback is undefined.

evanlucas commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

LGTM

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

thefourtheye commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

JungMinu commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM

cjihrig commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

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

cjihrig pushed a commit that referenced this pull request Nov 11, 2015
fs: return null error on readFile() success
1594198 
This commit ensures that readFile() callsback with a null
error consistently on success.

PR-URL: #3740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

cjihrig commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

Thanks! Landed in 1594198

cjihrig closed this Nov 11, 2015

cjihrig commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

@jasnell sorry, which labels should I apply to have this backported? There are too many labels to pick from :-)

jasnell commented Nov 11, 2015

Copy link
Copy Markdown
Member

Please use the lts-watch labels to identify things that should be back ported.

zbinlin deleted the fix-bug-branch branch November 12, 2015 04:37
rvagg pushed a commit that referenced this pull request Nov 13, 2015
fs: return null error on readFile() success
2aa6a6d 
This commit ensures that readFile() callsback with a null
error consistently on success.

PR-URL: #3740
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

MylesBorins commented Nov 17, 2015

Copy link
Copy Markdown
Contributor

removing this from lts-watch as it effects code that was part of a semver-major fix that will not be applied to v4.x

@cjihrig please feel free to reapply to tag if I am mistaken

rvagg mentioned this pull request Dec 17, 2015
Closed
MylesBorins mentioned this pull request Jan 14, 2016
Closed

rvagg commented Jan 15, 2016

Copy link
Copy Markdown
Member

agreeing with @thealphanerd on this and applying dont-land-on-v4.x, looks like prior to #3485 it was returning null already.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL