fs: name anonymous functions #9252
Conversation
nodejs-github-bot
added
the
fs
|
@7coder7 You may have to fix the commit. It says that it was made on May 25th. |
|
@thefourtheye is it fixed now? |
|
@7coder7 Yes. It looks fine now. But, it looks like there are test failures. |
|
@thefourtheye had to change the test cases |
| @@ -20,7 +20,7 @@ if (process.argv[2] === 'child') { | |||
| execFile(process.execPath, args, function(err, stdout, stderr) { | |||
| assert.equal(err, null); | |||
| assert.equal(stdout, ''); | |||
| if (/WARNING[\s\S]*fs\.readFileSync/.test(stderr)) | |||
| if (/WARNING[\s\S]*readFileSync/.test(stderr)) | |||
There was a problem hiding this comment.
Naming anonymous functions produces a stacktrace with just the function name now. For example, earlier it used to be fs.readFileSync and now it has become just readFileSync in the stacktrace.
Is that okay @nodejs/collaborators ?
There was a problem hiding this comment.
Maybe it's possible to modify Environment::PrintSyncTrace to get it back ?
It's not affecting the stack traces generated by V8 when an error is thrown.
There was a problem hiding this comment.
I'm fine with this but @targos' suggestion is good also.
There was a problem hiding this comment.
@targos I think v8 doesn't provide a way to get the actually bound object name. Is there any APIs which we can leverage? Also, PrintSyncTrace has only references to the Stack Frames. They just give only the debug function names.
There was a problem hiding this comment.
@thefourtheye yeah it seems that the API doesn't give us access to more information. I'm fine with the change too.
There was a problem hiding this comment.
Isn't the purpose of adding the names to improve stack trace? Maybe we ought to only add names in places where the stack trace can't determine the name? Like, anonymous callback functions, but not functions that are assigned to a property on a variable because the name is inferred from the assignment?
There was a problem hiding this comment.
Since the purpose of naming anonymous functions is to improve stack traces, and this seems to have the opposite effect, I'm actually -1 on this and similar changes, I think. I'm OK with this sort of thing in cases where the name cannot be inferred, but the name can be inferred in all the instances in this change, and the name that V8 infers is better than the name we provide in this change.
There was a problem hiding this comment.
I too have concerns about this. If fs.func turns to func, it is clearly a regression in my eyes. This can be particularly problematic in net and decendants where many functions are named the same. V8's function name inferring is bound to get better over time, too.
See #8913 (comment) for a suggestion.
| @@ -1779,7 +1779,7 @@ ReadStream.prototype.open = function() { | |||
| }); | |||
| }; | |||
|
|
|||
| ReadStream.prototype._read = function(n) { | |||
| ReadStream.prototype._read = function _read(n) { | |||
| if (typeof this.fd !== 'number') | |||
| return this.once('open', function() { | |||
There was a problem hiding this comment.
this is one that should be named.
|
Closing this since we decided not to pursue with this approach |
Checklist
make -j8 test(UNIX), orvcbuild test nosign(Windows) passesAffected core subsystem(s)
fs
Description of change
Ref: #8913