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

[MylesBorins]

This reverts commit 1124de2
which landed in #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

#5102 is waiting for review
but has not landed. We should revert ASAP to fix master while
we decide what to do.

/cc @nodejs/collaborators @rvagg @jasnell

[Fishrock123]

-1, the other patch LGTM

[MylesBorins]

@Fishrock123 I'm more than happy to close this if the other patch lands.

[MylesBorins]

I've gone ahead and cherry-picked my npm test from #5166 into this PR.

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

I think it makes sense to land this test to ensure that
a) the fix is working
b) we do not re create the regression this reversion is meant to fix

If this lands I'll close #5166

edit: i've gone ahead and closed #5166 in lieu of this PR, I'll reopen it if this does not land.

edit: CI is completely green!!!

[vkurchatkin]

LGTM. We should remember this as one more example of irresponsible package authors preventing core evolution

[ChALkeR]

I'm -1 on reverting that change.

We should not keep fs in that state forever, if people are re-evaluating it — it's a problem in the user code, not in Node.js. This has been previosly discussed already, and other core modules are not re-evaluatable (pretty much everything that uses internals). Why should fs be special? Also note that graceful-fs was fixed long ago, all what users have to do is to upgrade their deps.

[MylesBorins]

I think this could be an interested case study for @nodejs/ctc to come up with some process around.

While I agree that we cannot be held responsible for things being broken in user land I very strongly believe it is our responsibility to keep master working. If a commit that handles a deprecation is breaking master, a broken npm / gyp is a broken master imho, that commit should be reverted until it no longer does so.

These issues are tangential. Anything breaking master that cannot be quickly resolved should be reverted, no matter what the commit is. We are now at over 100 hours that master has been broken, which does not seem very reasonable to me.

[trevnorris]

I don't appreciate the fact that we'd have to revert this change either, but until we have a working solution not being able to build native modules with node-gyp on master is development life a pain. Unless another fix can be landed soon, it should be reverted. Once another fix has landed, then this can be applied again.

[ChALkeR]

@trevnorris Agreed on that. If the reverting is temporary until the better fix is found (hopefully soon), and the original commit will get back in before 6.0 comes out, I'm fine with reverting.

[rvagg]

lgtm as a temporary measure with the expectation that the commit in question will be back on master as soon as we have npm sorted out.

[MylesBorins]

Closing as the CTC has opted for landing #5102