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

backport: 11665 to v7 #12614

Closed
wants to merge 2 commits into from
Closed

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Apr 24, 2017

This is a backport of #11665 + a few non-semver-major changes (specifically only to fs.realpathSync() and fs.realpath()) from #10789.

#11665 was initially labeled as semver-major by me as I typically like to err on the side of caution because of the nature of some of the changes. However, some have expressed interest in downgrading it to a semver-minor to allow (a more straightforward backport of) a necessary fix for a discrepancy that exists currently in v7.x because of the way fs.Stats values are generated for fs.stat() vs fs.statSync() for example.

Landing this should fix the issue described in #12419.

/cc @nodejs/ctc

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • fs

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. v7.x labels Apr 24, 2017
@mscdex
Copy link
Contributor Author

mscdex commented Apr 24, 2017

@evanlucas evanlucas force-pushed the v7.x-staging branch 2 times, most recently from d4ceb59 to 69a8053 Compare May 3, 2017 12:56
mscdex added 2 commits May 4, 2017 16:18
Including:

* Move async *stat() functions to FillStatsArray() now used by the
sync *stat() functions

* Avoid creating fs.Stats instances for implicit async/sync *stat()
calls used in various fs functions

* Store reference to Float64Array data on C++ side for easier/faster
access, instead of passing from JS to C++ on every async/sync *stat()
call
Including:

* Skip URL instance check for common (string) cases

* Avoid regexp on non-Windows platforms when parsing the root of a path

* Skip call to `getOptions()` in common case where no `options` is passed

* Avoid `hasOwnProperty()`
@mscdex
Copy link
Contributor Author

mscdex commented May 4, 2017

CI again: https://ci.nodejs.org/job/node-test-pull-request/7867/
CITGM again: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/761/

/cc @nodejs/collaborators

It'd be nice to get this landed soon if everyone agrees/approves of the changes since it fixes potential issues with stat() values on some platforms.

@mscdex
Copy link
Contributor Author

mscdex commented May 26, 2017

ping @nodejs/collaborators

@mscdex
Copy link
Contributor Author

mscdex commented Jun 16, 2017

Closing this as AFAIK node v7.x is now an unsupported branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants