fs: deprecate fs.read's string interface by thefourtheye · Pull Request #4525 · nodejs/node (original) (raw)

fs.read supports a deprecated string interface version, which is
not documented. It was intended to be deprecated in this commit in 2010
c93e0aa
This patch issues a deprecation message saying the usage of this
interface is deprecated.

thefourtheye 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

Jan 4, 2016

@jasnell Can you please have a look at it again? I missed the readSync part. I force pushed that as well now.

heh... I forgot about that one too.. still LGTM but I'd like to get other opinions before landing

Can we add a suggested course of action to the deprecation message? Something like "Use the Buffer API instead" and maybe a link to the relevant docs.

1 similar comment

fs.read supports a deprecated string interface version, which is not documented. It was intended to be deprecated in this commit in 2010 nodejs@c93e0aa This patch issues a deprecation message saying the usage of this interface is deprecated.

@nodejs/collaborators I'll land this tomorrow if there are no objections.

thefourtheye added a commit that referenced this pull request

Feb 5, 2016

fs.read supports a deprecated string interface version, which is not documented. It was intended to be deprecated in this commit in 2010 c93e0aa This patch issues a deprecation message saying the usage of this interface is deprecated.

PR-URL: #4525 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Trevor Norris trev.norris@gmail.com Reviewed-By: Сковорода Никита Андреевич chalkerx@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl

Thanks for the reviews. Landed in 1124de2

This commit has broken npm on master.

To reproduce simply try to run npm install

Thankfully npm substack, npm visnup, and npm xmas are unaffected

Npm easter eggs aside I think that this commit should be backed out of master until a solution is found.

Unfortunate that we're tied down to a function signature that isn't even documented anymore.

We tried to figure out the actual problem and our findings are at #5097 (comment)

So, this wasn't causing the problem with npm after all?
On Fri, Feb 5, 2016 at 02:13 thefourtheye notifications@github.com wrote:

We tried to figure out the actual problem and our findings are at #5097
(comment)
#5097 (comment)

—
Reply to this email directly or view it on GitHub
#4525 (comment).

Short term proposed solution: back this out completely on v5.x and force-push. We need that branch to be stable and happy for our releases this week and I've got no qualms with force pushing to non-master branches, does anyone here object? We do it to v4.x-staging occasionally as we run into trouble there.

@rvagg This commit hasn't landed in 5.x yet. So that should be fine. Should we tag this as do-not-land-5.x or something?

@rvagg That got to 5.x? I was talking about master only. If that got into 5.x, I do not object force-pushing to revert it. But wasn't it semver-major?

@thefourtheye is correct, this is not in v5.x

If the fix is still giving people goosebumps and it not going to quickly land on master I'd like to see this change backed out so that we can have a working copy of npm on master... not having it is making smoke testing a bit of a pain (and arguably less reliable as it is running off an fork)

k, sorry, ignore my comment then!

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request

Feb 9, 2016

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.

ChALkeR added a commit to ChALkeR/io.js that referenced this pull request

Feb 11, 2016

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.

jasnell pushed a commit that referenced this pull request

Feb 13, 2016

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. PR-URL: #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

stefanmb pushed a commit to stefanmb/node that referenced this pull request

Feb 23, 2016

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

scovetta pushed a commit to scovetta/node that referenced this pull request

Apr 2, 2016

PR-URL: nodejs#4525 Reviewed-By: James M Snell jasnell@gmail.com Reviewed-By: Trevor Norris trev.norris@gmail.com Reviewed-By: Сковорода Никита Андреевич chalkerx@gmail.com Reviewed-By: Ben Noordhuis info@bnoordhuis.nl

jasnell added a commit that referenced this pull request

Apr 26, 2016