| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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. |
Sorry, something went wrong.
|
Hmmmm, would it be possible to use process.bindings('natives') ourselves to load the internal code? Arguably it'd be more robust against changes? |
Sorry, something went wrong.
There was a problem hiding this comment.
msg.startsWith(prefix) is logically always true, isn't it? Also, I wouldn't use .replace(), I'd use .slice(prefix.length).
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
I would. I'd fix up internal/util while you're at it (in a separate commit.)
Sorry, something went wrong.
There was a problem hiding this comment.
@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.
Sorry, something went wrong.
|
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? |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Done.
Sorry, something went wrong.
|
LGTM with a nit |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@rvagg Why was a semver-major commit landed on v5.x? |
Sorry, something went wrong.
|
sorry, wires crossed, ignore my comment |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
LGTM. Should we have a tracking issue to have the npm's dependency upgradation? |
Sorry, something went wrong.
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.
|
LGTM if it works and Ci is happy |
Sorry, something went wrong.
Sorry, something went wrong.
|
Also worth mentioning that I just submitted #5166 which tests the edge case this broke (re: npm). |
Sorry, something went wrong.
|
@ChALkeR feel free to cherry-pick my test 😄 : |
Sorry, something went wrong.
|
-1. this is just ridiculous |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
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>
|
Correct me if I'm wrong, but shouldn't this be dont-land-on-v5.x rather than semver-major? |
Sorry, something went wrong.
|
@thealphanerd This is not semver-major itself, but it depends on a semver-major commit (and adds a temporary fix on top of it). |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
@adambuczynski
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.
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.). |
Sorry, something went wrong.
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 :)
Ok I didn't know that, yes fair enough. |
Sorry, something went wrong.
|
@adambuczynski
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. |
Sorry, something went wrong.
You can use --trace-deprecation to get a stack trace, when you are running an application. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Not really, when I build Atom I just run script/build, and I don't think it supports node arguments.
Let's hope it works and the messages are banished from our world within a week or two :) |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Help Me to solve this problem.
|
Sorry, something went wrong.
|
@maniJoe Post questions at https://github.com/nodejs/help/issues, don't spam random issues. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.