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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Sep 23, 2016
Merged

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

merged 1 commit into from
Sep 23, 2016

Conversation

bnoordhuis
Copy link
Member

Blocked by #6413.

@bnoordhuis bnoordhuis added the fs Issues and PRs related to the fs subsystem / file system. label Jun 5, 2016
@mscdex
Copy link
Contributor

mscdex commented Jun 5, 2016 •
edited
Loading

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

@ChALkeR
Copy link
Member

ChALkeR commented Jun 5, 2016 •
edited
Loading

@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
Copy link
Member

ChALkeR commented Jun 5, 2016 •
edited
Loading

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 PRs that contain breaking changes and should be released in the next major version. label Jun 5, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jun 5, 2016

LGTM after #6413 lands (if CI passes).

@cjihrig
Copy link
Contributor

cjihrig commented Jun 6, 2016

LGTM once unblocked.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

LGTM

@Fishrock123
Copy link
Contributor

This isn't useful to expose?

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

It hasn't been exposed up to this point.

@jasnell jasnell added the blocked PRs that are blocked by other issues or PRs. label Jul 6, 2016
@ChALkeR ChALkeR mentioned this pull request Aug 9, 2016
Closed
2 tasks
ChALkeR referenced this pull request in isaacs/node-graceful-fs Aug 16, 2016
@isaacs
add support for node v7
07701dc 
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.
This was referenced Aug 17, 2016
Closed
Closed
@ChALkeR ChALkeR removed the blocked PRs that are blocked by other issues or PRs. label Aug 29, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Aug 29, 2016

#6413 landed.

@ChALkeR
Copy link
Member

ChALkeR commented Aug 31, 2016

/ping @bnoordhuis =)

@bnoordhuis
Copy link
Member Author

Rebased. New CI run: https://ci.nodejs.org/job/node-test-pull-request/4010/

@ChALkeR
Copy link
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.

@bnoordhuis
fs: move stringToFlags() to lib/internal
a8d2c9d 
PR-URL: nodejs#7162
Refs: nodejs#6413
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
@bnoordhuis bnoordhuis closed this Sep 23, 2016
@bnoordhuis bnoordhuis deleted the unexport-fs-stringtoflags branch September 23, 2016 16:26
@bnoordhuis bnoordhuis merged commit a8d2c9d into nodejs:master Sep 23, 2016
@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

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

@ChALkeR
Copy link
Member

ChALkeR commented Oct 10, 2016 •
edited
Loading

+1 to landing in v7.0

@Fishrock123
Copy link
Contributor

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

@ChALkeR
Copy link
Member

ChALkeR commented Oct 10, 2016 •
edited
Loading

@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 pushed a commit that referenced this pull request Oct 10, 2016
@bnoordhuis @jasnell
fs: move stringToFlags() to lib/internal
7f7d1d3 
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 mentioned this pull request Jun 15, 2017
Closed
3 tasks
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. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@bnoordhuis @mscdex @ChALkeR @cjihrig @jasnell @Fishrock123