fs: add a temporary fix for re-evaluation support #5102

ChALkeR · 2016-02-05T17:07:23Z

This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module.

To be reverted in Node.js 7.0.

Fixes #5097, see also #1898, #2026, and #4525.

ChALkeR · 2016-02-05T17:08:51Z

This does not cancel the fact that npm has to update graceful-fs inside it, but this should give a grace period to other users of graceful-fs instead of suddenly breking their setups with a bogus error message in 6.0.

Fishrock123 · 2016-02-05T17:17:25Z

Hmmmm, would it be possible to use process.bindings('natives') ourselves to load the internal code? Arguably it'd be more robust against changes?

bnoordhuis · 2016-02-05T18:25:15Z

lib/fs.js

+    if (process.throwDeprecation)
+      throw new Error(msg);
+    else if (process.traceDeprecation)
+      console.trace(msg.startsWith(prefix) ? msg.replace(prefix, '') : msg);


msg.startsWith(prefix) is logically always true, isn't it? Also, I wouldn't use .replace(), I'd use .slice(prefix.length).

That code was directly copied from internal/util, it doesn't look optimal to me either.
I can make it a bit cleaner here, should I?

I would. I'd fix up internal/util while you're at it (in a separate commit.)

@bnoordhuis I made this a bit cleaner here, but I don't think that the changes for internal/util need to go into this PR. Perhaps a separate one would be better.

Trott · 2016-02-06T00:28:09Z

Is it possible and not-onerous to write a test for this? EDIT: Like, maybe something that confirms that a sensible-ish message is being provided when the situation arises?

jasnell · 2016-02-07T20:41:29Z

lib/fs.js

+  printDeprecation('fs: re-evaluating native modules sources is not ' +
+                   'supported. If you are using graceful-fs module, update ' +
+                   'it to a recent version.',
+                    false);


nit: perhaps reword a bit to, re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update to a more recent version.

jasnell · 2016-02-07T20:41:58Z

LGTM with a nit

rvagg · 2016-02-08T06:22:36Z

As per #4525 (comment) I'm suggesting we also go with just removing #4525 from v5.x for now to give us breathing room.

This solution is kind of gross but lgtm since there's no other good way around it as far as I can tell.

ChALkeR · 2016-02-08T06:35:06Z

@rvagg Why was a semver-major commit landed on v5.x?

rvagg · 2016-02-08T06:43:43Z

sorry, wires crossed, ignore my comment

ChALkeR · 2016-02-08T07:20:46Z

@Fishrock123

Hmmmm, would it be possible to use process.bindings('natives') ourselves to load the internal code? Arguably it'd be more robust against changes?

The warning (and an error in 7.0) is only thrown if it's not possible to load internal/util. If that code is re-executed where internal is allowed (including user code with --expose-internals flag) there would not be any warning or error.

thefourtheye · 2016-02-08T07:24:10Z

LGTM. Should we have a tracking issue to have the npm's dependency upgradation?

This reverts commit 1124de2 which landed in nodejs#4525 This commit has broken both npm + node-gyp How did it break npm? Deprecating fs.read's string interface includes `internal/util` Currently npm's dep tree includes an old version of graceful-fs that is monkey patching fs. As such everything explodes when trying to require `internal/util` nodejs#5102 is waiting for review but has not landed. We should revert ASAP to fix master while we decide what to do.

Fishrock123 · 2016-02-09T18:16:44Z

LGTM if it works and Ci is happy

MylesBorins · 2016-02-09T19:02:31Z

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

MylesBorins · 2016-02-09T19:03:45Z

Also worth mentioning that I just submitted #5166 which tests the edge case this broke (re: npm).

ChALkeR · 2016-02-09T19:07:06Z

@Trott, I have pending tests for this, but let's put those in a separate PR, because I am not yet sure with those and adding them would delay getting master fixed. I don't want getting #4525 reverted :-)

MylesBorins · 2016-02-09T19:08:35Z

@ChALkeR feel free to cherry-pick my test 😄 :
MylesBorins@91b4f79

vkurchatkin · 2016-02-09T19:09:27Z

-1. this is just ridiculous

ChALkeR · 2016-02-09T19:12:21Z

@thealphanerd, it's not directly related. Your test is for npm and it's good to have it long-term, mine is for old versions of graceful-fs-like code and will be reverted in 7.0. I would want both of those, separately.

MylesBorins · 2016-02-09T19:13:01Z

@vkurchatkin

-1. this is just ridiculous

ok, so now we don't have consensus on this PR, and I don't imagine we are getting it soon. Can we please revert the original PR and reopen it so that we can fix master.

edit: I want to add that I do not think that reverting means the other should not land again, and potentially not land again quickly. master has been effectively broken since Friday, and I feel a sense of urgency to get things working again.

This is needed to give users a grace period before actually breaking modules that re-evaluate fs sources from context where internal modules are not allowed, e.g. older version of graceful-fs module. To be reverted in Node.js 7.0 Fixes: nodejs#5097, see also nodejs#1898, nodejs#2026, and nodejs#4525. PR-URL: nodejs#5102 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

Fishrock123 · 2016-03-30T20:45:35Z

Correct me if I'm wrong, but shouldn't this be dont-land-on-v5.x rather than semver-major?

ChALkeR · 2016-04-04T09:33:07Z

@Fishrock123 indeed.

ChALkeR · 2016-04-04T09:34:52Z

@thealphanerd This is not semver-major itself, but it depends on a semver-major commit (and adds a temporary fix on top of it).

bondden · 2016-05-03T11:39:42Z

Perhaps someone find it useful - not a solution, but a temporary walk-around for graceful-fs and alike modules: https://gist.github.com/bondden/2d2e07d18d94d1f4dc23b7dcf9b0e350

adamreisnz · 2016-05-23T00:32:31Z

@ChALkeR I'm not sure that the error message in this PR is very helpful to users. Almost every single app or node script that I run now displays this message.

For example, when building Atom:

Installing build modules...
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
(node:2342) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
=> Took 7025ms.

Or when running an Ember app:

(node:2792) fs: re-evaluating native module sources is not supported. If you are using the graceful-fs module, please update it to a more recent version.
WARNING: Node v6.2.0 has currently not been tested against Ember CLI and may result in unexpected behaviour.
version: 1.13.13
Livereload server on http://localhost:49152
Serving on http://localhost:4300/

The message doesn't show a stack trace, nor is it clear what module the problem is originating from, so there's no easy way to find out where to go to ask for a fix or what package to update.

Can this message be updated to be a bit more descriptive and maybe limit the amount of times it's shown? In it's current form, while it's not an error, it's not very useful and I feel it will still cause a lot of confusion amongst users.

ChALkeR · 2016-05-23T05:17:14Z

@adambuczynski

there's no easy way to find out where to go to ask for a fix or what package to update.

I haven't seen any real-world case when it wasn't caused by a graceful-fs 2.x or 3.x dependency. Everyone should just bump their graceful-fs dependencies to 4.x. You can use npm list graceful-fs to see why it was installed.

and maybe limit the amount of times it's shown?

It is already limited to 1 time per node process, unleast you have multiple old versions of graceful-fs installed, all of which should be updated. It looks reasonable to me to just print the message several times then, given that the code to track this further would introduce other undesired changes (storing a flag in another module, storing a flag in global scope, etc.).

adamreisnz · 2016-05-23T05:40:23Z

I haven't seen any real-world case when it wasn't caused by a graceful-fs 2.x or 3.x dependency. Everyone should just bump their graceful-fs dependencies to 4.x. You can use npm list graceful-fs to see why it was installed.

Ideally, yes everyone should do that. But by everyone you mean package maintainers, and package maintainers can only realistically be asked to maintain the dependencies of their own package.

I have the same gripe with this "error" message as with the "prefer global" warning that npm gives for nested packages (See npm/npm#11652)

End users or a package or script don't really care if some nested package of some dependency somewhere down the line in the dependency tree has a deprecated fs-graceful.

Basically I feel that this deprecation warning should be thrown by npm at install time rather than node at runtime, and only when you have the old version of fs-graceful in your dependencies, encouraging you to update your dependencies. I don't care what packages atom uses and what dependencies those packages have, I just want to be able to run the build script without seeing these ugly warnings all over the place :)

It is already limited to 1 time per node process, unleast you have multiple old versions of graceful-fs installed, all of which should be updated. It looks reasonable to me to just print the message several times then, given that the code to track this further would introduce other undesired changes (storing a flag in another module, storing a flag in global scope, etc.).

Ok I didn't know that, yes fair enough.

ChALkeR · 2016-05-23T05:59:49Z

@adambuczynski

Basically I feel that this deprecation warning should be thrown by npm at install time

It is thrown at both install time and in runtime. Just install time is not an option, because we can't guarantee that it's caused by graceful-fs in all cases, and a proper deprecation warning is needed here to make sure that users are notified of the future change.

thefourtheye · 2016-05-23T10:35:49Z

The message doesn't show a stack trace

You can use --trace-deprecation to get a stack trace, when you are running an application.

Fishrock123 · 2016-05-23T14:48:16Z

Note that those versions of graceful-fs will simply cause an error in future versions of node and refuse to run at all. The more people notice it, come ask questions / anything else, the better.

adamreisnz · 2016-05-23T19:01:15Z

You can use --trace-deprecation to get a stack trace, when you are running an application.

Not really, when I build Atom I just run script/build, and I don't think it supports node arguments.
Hence my point about end users having no use for those messages/errors and no easy way of dealing with them.

Note that those versions of graceful-fs will simply cause an error in future versions of node and refuse to run at all. The more people notice it, come ask questions / anything else, the better.

Let's hope it works and the messages are banished from our world within a week or two :)

Fishrock123 · 2016-05-23T19:29:01Z

Not really, when I build Atom I just run script/build, and I don't think it supports node arguments.

That's not really our fault. The flags for deprecations have been around for years. You should probably check, and if it is not capable of passing flags, we should probably prompt them for at least --trace-deprecation and --trace-warning to be made available.

adamreisnz · 2016-05-23T20:12:38Z

Of course, I'm not saying it is, but the point I'm trying to make is that even if it were available, I am not a developer or maintainer of Atom. As such, I don't care if their build script has deprecations or warnings because of the packages it (or its dependencies) use. I just want it to run and build the latest version of Atom for me without nagging me about the same deprecation 20 times :)

Those errors are good for developers / package maintainers to see and get warned about, not so much end users. I understand it's hard from a Node perspective to differentiate between the two, and that's why I thought it would be sufficient to issue deprecation warnings when a package installs the deprecated dependency.

However as ChALkeR mentioned that's not an option here, so we're kind of stuck with this message it seems.

maniJoe · 2017-02-09T12:58:48Z

Help Me to solve this problem.
I am unable to use angular IDE

npm ERR! Linux 4.4.0-63-generic
npm ERR! argv "/usr/bin/nodejs" "/usr/bin/npm" "install" "-g" "graceful-fs" "graceful-fs@latest-version"
npm ERR! node v4.2.6
npm ERR! npm v3.5.2

npm ERR! No compatible version found: graceful-fs@latest-version
npm ERR! Valid install targets:
npm ERR! 4.1.11, 4.1.10, 4.1.9, 4.1.8, 4.1.7, 4.1.6, 4.1.5, 4.1.4, 4.1.3, 4.1.2, 4.1.1, 4.1.0, 3.0.11, 3.0.10, 3.0.9, 3.0.8, 3.0.7, 3.0.6, 3.0.5, 3.0.4, 3.0.3, 3.0.2, 3.0.1, 3.0.0, 2.0.3, 2.0.2, 2.0.1, 2.0.0, 1.2.3, 1.2.2, 1.2.1, 1.2.0, 1.1.14, 1.1.13, 1.1.12, 1.1.11, 1.1.10, 1.1.9, 1.1.8, 1.1.7, 1.1.6, 1.1.5, 1.1.4, 1.1.3, 1.1.2, 1.1.1, 1.1.0, 1.0.2, 1.0.1, 1.0.0
npm ERR!
npm ERR!
npm ERR! If you need help, you may report this error at:
npm ERR! https://github.com/npm/npm/issues

npm ERR! Please include the following file with any support request:
npm ERR! /home/bosco/npm-debug.log

bnoordhuis · 2017-02-09T13:12:05Z

@maniJoe Post questions at https://github.com/nodejs/help/issues, don't spam random issues.

ChALkeR force-pushed the warn-fs-reevaluation branch 2 times, most recently from bce08a6 to 399137c Compare February 5, 2016 17:11

This was referenced Feb 5, 2016

fs: deprecate fs.read's string interface #4525

Closed

upgrade npm to 3.7.1 #5097

Closed

ChALkeR force-pushed the warn-fs-reevaluation branch from 399137c to 54ec1a8 Compare February 5, 2016 17:16

bnoordhuis reviewed Feb 5, 2016
View reviewed changes

Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Feb 5, 2016

jasnell reviewed Feb 7, 2016
View reviewed changes

ChALkeR force-pushed the warn-fs-reevaluation branch from 54ec1a8 to f9f35db Compare February 8, 2016 07:14

MylesBorins mentioned this pull request Feb 9, 2016

Revert "fs: deprecate fs.read's string interface" #5163

Closed

MylesBorins added the semver-major PRs that contain breaking changes and should be released in the next major version. label Feb 9, 2016

jasnell mentioned this pull request Feb 13, 2016

Tracking: Revert #5102 when possible #5213

Closed

jasnell closed this Feb 13, 2016

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

ChALkeR 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 4, 2016

Fishrock123 added the dont-land-on-v4.x label Apr 4, 2016

jasnell mentioned this pull request Apr 27, 2016

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

Closed

2 tasks

ChALkeR mentioned this pull request Apr 28, 2016

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

Closed

15 tasks

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

gagern mentioned this pull request Jul 8, 2016

Update to less 2 KaTeX/KaTeX#500

Closed

fs: add a temporary fix for re-evaluation support #5102

fs: add a temporary fix for re-evaluation support #5102

Conversation

ChALkeR commented Feb 5, 2016

ChALkeR commented Feb 5, 2016

Fishrock123 commented Feb 5, 2016

bnoordhuis Feb 5, 2016

Choose a reason for hiding this comment

ChALkeR Feb 5, 2016

Choose a reason for hiding this comment

bnoordhuis Feb 5, 2016

Choose a reason for hiding this comment

ChALkeR Feb 8, 2016

Choose a reason for hiding this comment

Trott commented Feb 6, 2016

jasnell Feb 7, 2016

Choose a reason for hiding this comment

ChALkeR Feb 8, 2016

Choose a reason for hiding this comment

jasnell commented Feb 7, 2016

rvagg commented Feb 8, 2016

ChALkeR commented Feb 8, 2016

rvagg commented Feb 8, 2016

ChALkeR commented Feb 8, 2016

thefourtheye commented Feb 8, 2016

Fishrock123 commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

ChALkeR commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

vkurchatkin commented Feb 9, 2016

ChALkeR commented Feb 9, 2016

MylesBorins commented Feb 9, 2016

Fishrock123 commented Mar 30, 2016

ChALkeR commented Apr 4, 2016

ChALkeR commented Apr 4, 2016

bondden commented May 3, 2016

adamreisnz commented May 23, 2016

ChALkeR commented May 23, 2016 • edited Loading

adamreisnz commented May 23, 2016

ChALkeR commented May 23, 2016

thefourtheye commented May 23, 2016

Fishrock123 commented May 23, 2016

adamreisnz commented May 23, 2016

Fishrock123 commented May 23, 2016 • edited Loading

adamreisnz commented May 23, 2016

maniJoe commented Feb 9, 2017

Help Me to solve this problem. I am unable to use angular IDE

bnoordhuis commented Feb 9, 2017

ChALkeR commented May 23, 2016 •

edited

Loading

Fishrock123 commented May 23, 2016 •

edited

Loading

Help Me to solve this problem.
I am unable to use angular IDE