Skip to content

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
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

@zbinlinzbinlin 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

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

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

@cjihrig

cjihrig commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

Care to change the var to let?

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

zbinlin commented Nov 10, 2015

Copy link
Copy Markdown
ContributorAuthor

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

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

evanlucas commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

LGTM

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

@thefourtheye

thefourtheye commented Nov 10, 2015

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment
@JungMinu

JungMinu commented Nov 10, 2015

Copy link
Copy Markdown
Member

LGTM

@cjihrig

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
@zbinlin@cjihrig
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

cjihrig commented Nov 11, 2015

Copy link
Copy Markdown
Contributor

Thanks! Landed in 1594198

@cjihrigcjihrig closed this Nov 11, 2015
@cjihrig

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

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
zbinlin deleted the fix-bug-branch branch November 12, 2015 04:37
rvagg pushed a commit that referenced this pull request Nov 13, 2015
@zbinlin@rvagg
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

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

@rvaggrvagg mentioned this pull request Dec 17, 2015
Closed
@MylesBorinsMylesBorins mentioned this pull request Jan 14, 2016
Closed
@rvagg

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.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

fsIssues 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

@zbinlin@cjihrig@evanlucas@thefourtheye@JungMinu@jasnell@MylesBorins@rvagg@mscdex