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

fs: move stringToFlags() to lib/internal #7162

Merged
merged 1 commit into from Sep 23, 2016

Conversation

Projects
None yet
6 participants
@bnoordhuis
Member

bnoordhuis commented Jun 5, 2016

Blocked by #6413.

@bnoordhuis bnoordhuis added the fs label Jun 5, 2016

@mscdex

This comment has been minimized.

Contributor

mscdex commented Jun 5, 2016

Are there any 3rd party modules that are using fs._stringToFlags()?

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 5, 2016

@mscdex Doesn't look like it (except for obvious copies of fs.js from Node.js, but those just define their own, and several copies of test-fs-open-flags.js which are likely never run).

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 5, 2016

This is still technically a semver-major change. It's fine though, because it depends on a semver-major change anyway.

@ChALkeR ChALkeR added the semver-major label Jun 5, 2016

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jun 5, 2016

LGTM after #6413 lands (if CI passes).

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jun 6, 2016

LGTM once unblocked.

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 6, 2016

LGTM

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Jun 6, 2016

This isn't useful to expose?

@jasnell

This comment has been minimized.

Member

jasnell commented Jun 6, 2016

It hasn't been exposed up to this point.

@jasnell jasnell added the blocked label Jul 6, 2016

@ChALkeR ChALkeR referenced this pull request Aug 9, 2016

Closed

Revert "fs: add a temporary fix for re-evaluation support" #6413

2 of 2 tasks complete

ChALkeR referenced this pull request in isaacs/node-graceful-fs Aug 16, 2016

add support for node v7
node version 7 officially deprecates `fs.read` and `fs.readSync`

This is done using `internal/util`. The unfortunate side affect
being that `graceful-fs v3.x` explodes when running the code in
`vm` as `internal/util` is not accessible from userland.

This commit uses a regular expression to replaces the require of
the specific internal util function with the source of that util
function. As such `graceful-fs v3.x` will no longer explode in
node v7.

One advantage to this approach is that any future deprecation
will not break graceful-fs.

@ChALkeR ChALkeR removed the blocked label Aug 29, 2016

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 29, 2016

#6413 landed.

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Aug 31, 2016

/ping @bnoordhuis =)

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:unexport-fs-stringtoflags branch Sep 11, 2016

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented Sep 11, 2016

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Sep 11, 2016

LGTM if CI passes.

@jasnell, note: this removes fs._stringToFlags without a deprecation, which has been present since long ago (0.8.x at least has it), but it was never documented and looks unused by anyone except for the fs.js and test-fs-open-flags.js copies.

@Trott Trott force-pushed the nodejs:master branch to c5ce7f4 Sep 21, 2016

fs: move stringToFlags() to lib/internal
PR-URL: #7162
Refs: #6413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:unexport-fs-stringtoflags branch to a8d2c9d Sep 23, 2016

@bnoordhuis bnoordhuis closed this Sep 23, 2016

@bnoordhuis bnoordhuis deleted the bnoordhuis:unexport-fs-stringtoflags branch Sep 23, 2016

@bnoordhuis bnoordhuis merged commit a8d2c9d into nodejs:master Sep 23, 2016

@jasnell

This comment has been minimized.

Member

jasnell commented Oct 10, 2016

@nodejs/ctc ... any objections to this landing in v7.x?

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Oct 10, 2016

+1 to landing in v7.0

@Fishrock123

This comment has been minimized.

Member

Fishrock123 commented Oct 10, 2016

Hmm, does this mean more patching to Graceful-fs?

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Oct 10, 2016

@Fishrock123 I don't think so. graceful-fs@1 and graceful-fs@4 do not re-evaluate fs sources, graceful-fs@2 is already broken in v7.0 and did not receive a fix, and graceful-fs@3 deployed a terrible hack that allows it to require internal/*.

A citgm run won't hurt, of course.

jasnell added a commit that referenced this pull request Oct 10, 2016

fs: move stringToFlags() to lib/internal
PR-URL: #7162
Refs: #6413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment