tools: don't use global Buffer and add linter rule for it

[silverwind]

This does a few things to the linter configuration:

• instead of relying on the predefined node environment, specify all our globals manually and as read-only.
• enable no-undef to error on undefined variable access.
• add globalReturn which allows to return from the top scope. This was implied by the node option before.

A number of tests intentionally use undefined globals to trigger an error or set globals (in the vm tests). I had to exclude those tests with an eslint-disable comment. If these comments are too noisy, we could alternatively disable that rule for all tests.

I might have missed a few globals from the v8 environment. Object.keys(global) doesn't list many of them, presumably because they are unenumerable. Any better ideas on how to discover globals?

cc: @Fishrock123@domenic

[silverwind]

@Fishrock123 what's the plan exactly with Buffer usage mentioned in #1770? Should it error as soon as Buffer is used inside lib?

[silverwind]

Same as above

[Fishrock123]

@Fishrock123 what's the plan exactly with Buffer usage mentioned in #1770? Should it error as soon as Buffer is used inside lib?

Probably? How else would it work?

[Fishrock123]

Perhaps disable for all tests?

[silverwind]

Yeah, I think it's better that way, less noisy comments all over the tests, will work something out.

[rlidwka]

no-undef is one of the few rules that do something good (as opposed to the most eslint rules that do nothing except for enforcing a religion code style). I don't think it should be disabled for all tests, because it could catch real errors.

What about explicitly declaring globals in every file?

/* globals: undefined_reference_error_maker */

I don't remember the exact syntax, but I think you can add globals on per-file basis. And most linters (jshint, eslint) respect that.

[silverwind]

I agree generally to using no-undef, but it is pretty common to use undefined globals in tests to trigger an error. With the rule enabled, one would have to research how to configure the linter when writing a test, which I'd like to avoid.

Using a global comment is pretty equivalent to disabling the rule, both require additional ugly comments. I think we're fine with just disabling no-undef in the test directory.

[rlidwka]

maybe ask people to trigger an error differently?

my favorite way is []()

[silverwind]

@Fishrock123 I'll just comment out the Buffer global in .eslintrc to indicate it's there but not allowed to be used directly. We want to use require('buffer').Buffer in place of it, right?

[Fishrock123]

@silverwind correct.

[silverwind]

Rebased and disabled no-undef for tests, also moved the rule to the correct category in .eslintrc.

@Fishrock123 There are 83 occasions where Buffer is used directly in lib. Should I go for it and replace them with a non-global?

[silverwind]

@Fishrock123 I'll follow up with a port of nodejs/node-v0.x-archive@523929c in another PR. Does this one LGTY?

[Fishrock123]

@silverwind I'd prefer if you did the commits in this order: fix tests, apply full remove-global-buffer patch, add linting.

That being said, this still doesn't seem to catch global Buffer use with ./iojs tools/eslint/bin/eslint.js src lib test --reset?

[silverwind]

Well I suppose I can split these up into multiple commits and add the Buffer thing too in its own commit.

[silverwind]

Alright, split up in 3 commits now. The circular dependency described in nodejs/node-v0.x-archive#8603 (comment) didn't happen, so no workaround was needed.

[silverwind]

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/728/

[silverwind]

Updated and cleaned up the list of globals, which is now sorted in categories. @Fishrock123 PTAL

[targos]

GLOBAL and root are going to be deprecated (#1838). Are they still being used in the code base ? If not they do not need to be here.

[thefourtheye]

But they ll still be there till removal, right?

[silverwind]

They're not used and no one sane would use them, so good call on removing them.

[silverwind]

Filed sindresorhus/globals#33 for the missing DataView builtin in eslint, until then we probably have to keep it listed for correctness (it's only used in one test).

[silverwind]

Also filed eslint/eslint#2657 for a simpler way to undefine globals.

[silverwind]

Don't review yet, I'll try creating a custom eslint rule for the Buffer case, which would drastically simplify the current changes to globals and also print a nice custom error message in the linter.

[silverwind]

Here's my current custom rule, which @xjamundx graciously provided:

'use strict';module.exports=function(context){varrequired=false;return{CallExpression: function(node){if(node.callee.type==='Identifier'&&node.callee.name==='require'){if(node.arguments.length&&node.arguments[0].value==='buffer'){required=true;}}},Identifier: function(node){if(!required&&node.name==='Buffer'){context.report(node,"Make sure to require('buffer').Buffer before using Buffer");}}}}

It's almost working, except that it does detect an error on the require line itself:

constBuffer=require('buffer').Buffer;// ^--- errors here

Any suggestions?

[rlidwka]

I think your suggestion in eslint/eslint#2657 is much better than this headache with custom rules. Maybe re-open it?

[silverwind]

Up for review again!

[trevnorris]

I'm realizing right now that unless we make the exports Buffer property read only it can still be overridden just as easily by users. So what's the point of forcing it to be required in all files?

[silverwind]

I don't see writable Buffer as an problem really. It could allow for a custom Buffer implementation and a lot of things in node can be overridden right now anyways.

As for the reason of this change, there's a series of issues/prs that lead to it:

nodejs/node-v0.x-archive#8588
nodejs/node-v0.x-archive#8603
#1770

[trevnorris]

Ah yes. I forgot about the REPL issue. Thanks for the reminder.

I don't see the same change here for assert.js as in the final commit in nodejs/node-v0.x-archive@523929c (using b.Buffer instead). Has that issue been fixed?

[silverwind]

Yes, I think so. Tests were passing so I assumed it was fixed.

[Fishrock123]

@trevnorris I looked at that while attempting to port the original patch... I'm not sure why that was done in the first place?

Oh, found it here: nodejs/node-v0.x-archive#8603 (comment) Looks like there was a circular dependancy issue?

[silverwind]

@Fishrock123@trevnorris@vkurchatkin I don't understand why this race condition happened last time, but it seems to be gone now, all tests are passing so I assume it must've been fixed in the meanwhile.

I rebased and did a minor correction to the require-buffer rule (switched useless some for forEach). I also caught two errors through no-undef that were introduced in 5795e83, fix for that in #1951.

[silverwind]

Did another small correction to the printed message. Please review.

[trevnorris]

I thought 'use strict' had to go on the first line of either the file or function. Does that exclude comments?

/cc @domenic

[trevnorris]

nm. see it doesn't matter.

[rlidwka]

I'm pretty sure it does exclude comments, considering the amount of files with copyright hats at the very top.

[trevnorris]

Have one nit, but LGTM.

[thefourtheye]

Same as above ^^

[silverwind]

I'll fix that.

[trevnorris]

Great thanks. I'm cool with this change.

[silverwind]

Thanks! Landed in ff8202c .. 6e4d302.