Skip to content

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Oct 7, 2019

This seems to make sense if we want to promote the use
of fs.promises, although it’s not strictly necessary.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@addaleax addaleax requested a review from jasnell October 7, 2019 18:29
@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. http2 Issues or PRs related to the http2 subsystem. labels Oct 7, 2019
@addaleax addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Oct 7, 2019
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 8, 2019
@nodejs-github-bot
Copy link
Collaborator

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't instanceof a bit slow? Maybe this world be better if the checks were the other way around?

Copy link
Member Author

Choose a reason for hiding this comment

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

This was changed on request by @cjihrig … tbh, I wouldn’t expect it to make much of a difference in the big picture. And at some point in the recent past instanceof also became faster than a direct prototype comparison (according to 147b9d9), so it’s probably not too slow.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@addaleax
Copy link
Member Author

addaleax commented Oct 9, 2019

Landed in 0ff4a55

@addaleax addaleax closed this Oct 9, 2019
addaleax added a commit that referenced this pull request Oct 9, 2019
This seems to make sense if we want to promote the use
of `fs.promises`, although it’s not strictly necessary.

PR-URL: #29876
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@addaleax addaleax deleted the http2-filehandle branch October 9, 2019 21:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fs Issues and PRs related to the fs subsystem / file system. http2 Issues or PRs related to the http2 subsystem. 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.

8 participants