Revert "fs: add a temporary fix for re-evaluation support" by jasnell · Pull Request #6413 · nodejs/node · GitHub

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

As planned, This reverts commit 1d79787.

Fixes: #5213
Refs: #5102

@nodejs/ctc

LGTM

LGTM.

LGTM

Has anyone tried running the npm in node master with this?

make test-npm passes on master with this. All clear.

@Fishrock123 eslint should also work, as it uses a newer version of graceful-fs.

@Fishrock123 ... yeah, I had tested it last night right before opening the PR. Everything appeared to be ok but if there's any doubt there's no harm in leaving this PR open for a bit until we're sure. I just didn't want it to go un-done.

Also, I marked this semver-major defensively. It probably does not need to be semver-major but it definitely likely should be don't land on v6 and below.

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

It probably does not need to be semver-major but it definitely likely should be don't land on v6 and below.

I agree, let's just use the other labels, unless we really think it should be in a v7 (probably more like LTS 3/ v8) changelog.

@nodejs/ctc ... are we ready to land this or no?

I'm a little hesitant, maybe for v8?

I'm definitely -1 on waiting until v8. The original decision was to revert this in v7. I have no issues letting this sit a while longer but definitely don't want to push it out that far (largely because there's simply no reason to)

@jasnell I will try to create a list of packages currently affected by this.

lgtm but don't see reason to hurry landing it to introduce code churn making cherry-picking harder

@rvagg One reason for landing this sooner than later would be so that people who test nightlies and ignored the deprecation message will actually notice things being broken if something still uses old graceful-fs versions.

The cherry-picking impact of this should be minimal and fairly easy to deal
with.
On May 17, 2016 5:37 AM, "Rod Vagg" notifications@github.com wrote:

lgtm but don't see reason to hurry landing it to introduce code churn
making cherry-picking harder

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#6413 (comment)

I'm +1 for unblocking but I'd rather land #8166 than this one.

@jasnell I believe we could do both, but the one that gets landed later should get manually rebased.
I don't see any reason for blocking any of those against each other.

Well, the other PR also reverts this but in a different way. If that one landed, there'd be no reason to also land this one.

@jasnell No, that one keeps it as a deprecation warning, while the idea here is to remove the warning and turn it into a throw.

We could either:

Land lib,fs: use process.emitWarning instead of internal print dep warning #8166, then make it a throw instead of a warning in a rebase of this PR.
Land this (turn warning into a throw), then rebase lib,fs: use process.emitWarning instead of internal print dep warning #8166 in a way so that it keeps the throw (instead of keeping the warning).

Ah.. I see what you're saying. I can update #8166 to make it a throw if that's what we want it to do.

@jasnell only if this happens to land first, becase that is a semver-major change that is unrelated to #8166. 😉

So, can we unblock this?

/cc @nodejs/ctc.

I am pretty sure this was blocked by gulp and graceful-fs@3 not being compatible. They are fine now.

I propose to unblock and land this for v7.0, we already have a bunch of other PRs blocked by this: #6749, #6573, #7162, #2025, #8277 #8292, and most part of those are active.

See current usage data in #6413 (comment).

Unblocking should be fine

Once again: LGTM =).

@nodejs/ctc ... because graceful-fs v3 has now been updated to avoid this issue, I'd like to go ahead and land this revert. Any objections?

lgtm

On Fri, Aug 26, 2016, 5:59 PM James M Snell notifications@github.com
wrote:

@nodejs/ctc https://github.com/orgs/nodejs/teams/ctc ... because
graceful-fs v3 has now been updated to avoid this issue, I'd like to go
ahead and land this revert. Any objections?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#6413 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV4DpsFHJh9DBobqEZIZ4W26CvlE-ks5qjyl4gaJpZM4IQnMH
.

@nodejs/ctc ... if there are no objections, I will land this on Monday.

LGTM

Still LGTM.

New CI before landing: https://ci.nodejs.org/job/node-test-pull-request/3874/

Red in the last run, seemingly unrelated but just in case: https://ci.nodejs.org/job/node-test-pull-request/3875/

As planned, This reverts commit 1d79787. Fixes: #5213 PR-URL: #6413 Reviewed-By: Ben Noordhuis <info@noordhuis.nl> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: JungMinu - Minwoo Jung <jmwsoft@gmail.com> Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: Myles Borins <myles.borins@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Landed (finally) in 49ef3ae

At long last =)

@jasnell, thanks for taking care of this!

As an alternative to nodejs#6413, use process.emitWarning() instead of the internal printDeprecationMessage in order to avoid use of an internal only API.

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>

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>

This upgrades the transitive dep graceful-fs@3 to v4 which gets around the breakage caused by: nodejs/node#6413

jasnell added 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. labels Apr 27, 2016

Fishrock123 added this to the 7.0.0 milestone Apr 27, 2016

ChALkeR mentioned this pull request Apr 28, 2016

[epic] vinyl-fs version 3.0.0 gulpjs/gulp#1604

Closed

15 tasks

Fishrock123 added dont-land-on-v5.x and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 30, 2016

This was referenced May 5, 2016

Update glob for node 6 compatibility mochajs/mocha#2234

Closed

Update deprecation warning s/6/7/ isaacs/node-graceful-fs#64

Closed

jasnell added the ctc-agenda label May 16, 2016

Fishrock123 added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 17, 2016

ChALkeR mentioned this pull request May 18, 2016

fs: move SyncWriteStream to internal/fs #6749

Closed

3 tasks

addaleax mentioned this pull request Aug 26, 2016

No abort on require() with null bytes #8277

Closed

ChALkeR removed the blocked PRs that are blocked by other issues or PRs. label Aug 26, 2016

rvagg mentioned this pull request Aug 27, 2016

fs: move nullCheck to internal/fs.js to dedupe #8292

Closed

jasnell closed this Aug 29, 2016

Tyriar added a commit to Tyriar/gulp-untar that referenced this pull request Oct 23, 2016

Uplevel tar dep to v1

ae3563d

This upgrades the transitive dep graceful-fs@3 to v4 which gets around the breakage caused by: nodejs/node#6413

Tyriar mentioned this pull request Oct 23, 2016

Uplevel tar dep to v1 jmerrifield/gulp-untar#7

Merged

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

Revert "fs: add a temporary fix for re-evaluation support" by jasnell · Pull Request #6413 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

Revert "fs: add a temporary fix for re-evaluation support"#6413jasnell wants to merge 1 commit intonodejs:masternodejs/node:masterfrom jasnell:revert-5102jasnell/node:revert-5102Copy head branch name to clipboard

Conversation

jasnell commented Apr 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis commented Apr 27, 2016

Uh oh!

ChALkeR commented Apr 27, 2016

Uh oh!

JungMinu commented Apr 27, 2016

Uh oh!

Fishrock123 commented Apr 27, 2016

Uh oh!

Fishrock123 commented Apr 27, 2016

Uh oh!

ChALkeR commented Apr 27, 2016

Uh oh!

jasnell commented Apr 27, 2016

Uh oh!

jasnell commented Apr 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Apr 29, 2016

Uh oh!

Fishrock123 commented Apr 30, 2016

Uh oh!

jasnell commented May 16, 2016

Uh oh!

Fishrock123 commented May 16, 2016

Uh oh!

jasnell commented May 17, 2016

Uh oh!

ChALkeR commented May 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rvagg commented May 17, 2016

Uh oh!

ChALkeR commented May 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented May 17, 2016

Uh oh!

jasnell commented Aug 25, 2016

Uh oh!

ChALkeR commented Aug 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 25, 2016

Uh oh!

ChALkeR commented Aug 25, 2016

Uh oh!

jasnell commented Aug 25, 2016

Uh oh!

ChALkeR commented Aug 25, 2016

Uh oh!

ChALkeR commented Aug 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 26, 2016

Uh oh!

ChALkeR commented Aug 26, 2016

Uh oh!

jasnell commented Aug 26, 2016

Uh oh!

MylesBorins commented Aug 26, 2016

Uh oh!

jasnell commented Aug 27, 2016

Uh oh!

cjihrig commented Aug 27, 2016

Uh oh!

bnoordhuis commented Aug 27, 2016

Uh oh!

jasnell commented Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Revert "fs: add a temporary fix for re-evaluation support"#6413
jasnell wants to merge 1 commit into
nodejs:masterfrom
jasnell:revert-5102

jasnell commented Apr 27, 2016 •
edited
Loading

jasnell commented Apr 27, 2016 •
edited
Loading

ChALkeR commented May 17, 2016 •
edited
Loading

ChALkeR commented May 17, 2016 •
edited
Loading

ChALkeR commented Aug 25, 2016 •
edited
Loading

ChALkeR commented Aug 26, 2016 •
edited
Loading

jasnell commented Aug 29, 2016 •
edited
Loading

ChALkeR commented Aug 29, 2016 •
edited
Loading