Skip to content

Commit

Permalink
add support for node v7
Browse files Browse the repository at this point in the history
node version 7 officially deprecates `fs.read` and `fs.readSync`

This is done using `internal/util`. The unfortunate side affect
being that `graceful-fs v3.x` explodes when running the code in
`vm` as `internal/util` is not accessible from userland.

This commit uses a regular expression to replaces the require of
the specific internal util function with the source of that util
function. As such `graceful-fs v3.x` will no longer explode in
node v7.

One advantage to this approach is that any future deprecation
will not break graceful-fs.
  • Loading branch information
Myles Borins authored and isaacs committed Aug 12, 2016
1 parent 45c57aa commit 07701dc
Showing 1 changed file with 21 additions and 0 deletions.
21 changes: 21 additions & 0 deletions fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,27 @@ var mod = require("module")
var pre = '(function (exports, require, module, __filename, __dirname) { '
var post = '});'
var src = pre + process.binding('natives').fs + post
var deprecation = ''

var printDeprecation = ['var prefix = \'(\' + [process.release.name, process.pid].join(\':\') + \')\';',
'var printDeprecation = function(msg, warned) {',
' if (process.noDeprecation)',
' return true;',
' if (warned)',
' return warned;',

This comment has been minimized.

Copy link
@jdalton

jdalton Aug 16, 2016

The source has
if (warned || process.noDeprecation) return true
which is different than
if (warned) return warned;

' if (process.throwDeprecation)',
' throw new Error(prefix + msg);',
' else if (process.traceDeprecation)',
' console.trace(msg);',
' else',
' console.error(prefix + msg);',
' return true;',
'};'].join('\n');

var deprecrationRequire = /const printDeprecation = require\(\'internal\/util\'\).printDeprecationMessage;/

src = src.replace(deprecrationRequire, printDeprecation);

var vm = require('vm')
var fn = vm.runInThisContext(src)
fn(exports, require, module, __filename, __dirname)

10 comments on commit 07701dc

@phated
Copy link

@phated phated commented on 07701dc Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎆

@ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented on 07701dc Aug 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to work, at least not in this state. Or it will need several more graceful-fs@3 patches to work with v7, and would need to be constantly updated to keep up with minor or patch Node.js versions.

The deprecation message is not what is wrong here, graceful-fs@2 and graceful-fs@3 re-evaluating Node.js sources is what is wrong here.

nodejs/node#6749, nodejs/node#6573, and nodejs/node#7162 are going to break graceful-fs@3 again, even with this patch being landed.

Note that re-evaluating sources is not supported, and similar commits that use internal stuff could be landed (and afaik were already landed for other core modules) in minor or patch versions. Anything like that is going to break graceful-fs@3 and everything that depends on it again, and that already happened earlier.

The only way to fix the problem here is to stop using graceful-fs@2 and graceful-fs@3.

@isaacs
Copy link
Owner

@isaacs isaacs commented on 07701dc Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize that this almost certainly has been discussed already, but could someone point me to an explanation somewhere for the benefits of moving code to be inaccessible by userland JavaScript programs?

The guiding principles of this project explicitly state that "Change just for the sake of change must be avoided" and that there's a goal of "deferring, as much as possible, additional capabilities and features to add-on modules or applications".

What value of the project is served by these sorts of architectural changes? It seems like it disrupts extant userland programs while also reducing the ability to defer additional capabilities to add-on modules or applications, so I'd expect that there's some very significant benefit to make that worthwhile.

@phated
Copy link

@phated phated commented on 07701dc Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't like that comment enough

@ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented on 07701dc Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaacs, could we move this discussion to nodejs/node repo, where it belongs? More people would be able to notice it there.

@isaacs
Copy link
Owner

@isaacs isaacs commented on 07701dc Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChALkeR Happy to! Should I open a new issue?

@ChALkeR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaacs I suppose so. I can't even cc the CTC here =).

@phated
Copy link

@phated phated commented on 07701dc Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaacs
Copy link
Owner

@isaacs isaacs commented on 07701dc Aug 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ChALkeR
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@isaacs, thanks =).

Please sign in to comment.