Add restricted globals package #2286

sidoshi · 2017-05-20T11:44:35Z

Add restricted globals in a separate package.

gaearon · 2017-05-20T14:59:39Z

packages/eslint-restricted-globals/.gitignore

@@ -0,0 +1 @@
+node_modules


Shouldn't we have it in top level gitignore?

gaearon · 2017-05-20T15:00:46Z

packages/eslint-restricted-globals/README.md

+        console.log(name)
+    }
+}
+```


Let's use this example:

function printStatus(stats) { console.log(status); }

gaearon · 2017-05-20T15:01:11Z

packages/eslint-restricted-globals/index.js

@@ -0,0 +1,60 @@
+module.exports = [


Please add use strict and standard license header.

gaearon · 2017-05-20T15:01:21Z

packages/eslint-restricted-globals/package.json

@@ -0,0 +1,21 @@
+{
+  "name": "eslint-restricted-globals",
+  "version": "0.1.1",


Let's make it 1.0

gaearon · 2017-05-20T15:03:00Z

packages/eslint-restricted-globals/yarn.lock

@@ -0,0 +1,187 @@
+# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Let's remove the lock here.

gaearon · 2017-05-20T15:03:26Z

packages/eslint-restricted-globals/package.json

@@ -0,0 +1,21 @@
+{
+  "name": "eslint-restricted-globals",


Let's call it confusing-browser-globals.

gaearon · 2017-05-20T15:04:32Z

packages/eslint-restricted-globals/test.js

+let globals = require('./');
+
+it('should return an Array of globals', function() {
+  assert.strictEqual(globals.constructor, Array);


Let's instead test that it contains event as a sanity check.

gaearon · 2017-05-20T15:04:52Z

packages/eslint-restricted-globals/package.json

+  "description": "A list of confusing globals that should be restricted to be used as globals",
+  "main": "index.js",
+  "scripts": {
+    "test": "mocha"


Let's make sure this test runs on our CI.

Not sure how to do this. 😕

gaearon · 2017-05-20T16:44:09Z

packages/confusing-browser-globals/README.md

@@ -0,0 +1,42 @@
+# eslint-restricted-globals [![npm](https://img.shields.io/npm/v/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals) [![npm](https://img.shields.io/npm/l/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals)


Need to update this too.

gaearon · 2017-05-20T16:44:13Z

packages/confusing-browser-globals/README.md

+## Install
+
+```
+$ npm install --save eslint-restricted-globals


gaearon · 2017-05-20T16:44:20Z

packages/confusing-browser-globals/README.md

+For eg:
+```js
+function logStats(stats) {
+    console.log(status)


Two spaces please.

gaearon · 2017-05-20T16:44:35Z

packages/confusing-browser-globals/README.md

+var restrictedGlobals = require('eslint-restricted-globals')
+
+module.exports = {
+    rules: {


Please use two spaces for indentation.

gaearon · 2017-05-20T16:44:43Z

packages/confusing-browser-globals/README.md

+Add this in your eslint config in rules property:
+
+```js
+var restrictedGlobals = require('eslint-restricted-globals')


Please use semicolons.

gaearon · 2017-05-20T16:45:14Z

packages/confusing-browser-globals/test.js

+let globals = require('./');
+
+it('should return an Array of globals', function() {
+  assert.strictEqual(globals.constructor, Array);


Nit: please use Array.isArray() instead.

gaearon · 2017-05-20T19:05:45Z

packages/confusing-browser-globals/README.md

@@ -1,11 +1,11 @@
-# eslint-restricted-globals [![npm](https://img.shields.io/npm/v/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals) [![npm](https://img.shields.io/npm/l/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals)
+# eslint-restricted-globals


The title is still old.

Sorry, UPDATING.

gaearon · 2017-05-20T19:06:46Z

packages/confusing-browser-globals/test.js

@@ -4,7 +4,7 @@ let assert = require('assert');
 let globals = require('./');

 it('should return an Array of globals', function() {
-  assert.strictEqual(globals.constructor, Array);
+  assert.strictEqual(Array.isArray(globals), true);


Actually, would you mind changing this to use Jest for testing?

We don't use it in integration tests because of some obscure jsdom issue but it would be great to dogfood it in other places.

Okay, but I don't know how to work with CI tests.

I just mean that instead of calling mocha you could call jest (and add it to dependencies).

And replace assertions with Jest syntax:

expect(Array.isArray(globals)).toBe(true);

* Add restricted globals package * Use new package in eslint-config * Add eslint-restricted-globals dependency * Fixes * Update dependencies * Update test and README * Use jest * tweaks * Add lint/test to CI * Fix lint

facebook-github-bot added the CLA Signed label May 20, 2017

sidoshi mentioned this pull request May 20, 2017

Blacklist confusing global variables #2130

Merged

gaearon reviewed May 20, 2017

View reviewed changes

packages/eslint-restricted-globals/.gitignore Outdated

@@ -0,0 +1 @@

node_modules

Copy link

Contributor

gaearon May 20, 2017

Choose a reason for hiding this comment

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

Shouldn't we have it in top level gitignore?

gaearon reviewed May 20, 2017

View reviewed changes

gaearon suggested changes May 20, 2017

View reviewed changes

gaearon added this to the 1.0.2 milestone May 20, 2017

gaearon added the tag: internal label May 20, 2017

gaearon modified the milestones: 1.0.x, 1.0.2 May 20, 2017

gaearon reviewed May 20, 2017

View reviewed changes

sidoshi and others added 8 commits January 14, 2018 01:01

Add restricted globals package

fb3a416

Use new package in eslint-config

bd813b7

Add eslint-restricted-globals dependency

ba89e2b

Fixes

0464c48

Update dependencies

6619ef0

Update test and README

3930212

Use jest

6974150

tweaks

ee337a6

gaearon changed the base branch from master to next January 14, 2018 01:16

gaearon modified the milestones: 1.0.x, 2.0.0 Jan 14, 2018

gaearon approved these changes Jan 14, 2018

View reviewed changes

gaearon added 2 commits January 14, 2018 01:20

Add lint/test to CI

4e0e8d3

Fix lint

6043a08

gaearon merged commit 6f8e9b8 into facebook:next Jan 14, 2018

edmorley mentioned this pull request May 8, 2018

eslint-restricted-globals vs confusing-browser-globals sidoshi/eslint-restricted-globals#2

Closed

lock bot locked and limited conversation to collaborators Jan 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restricted globals package #2286

Add restricted globals package #2286

sidoshi commented May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

sidoshi May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

gaearon May 20, 2017

sidoshi May 20, 2017

gaearon May 20, 2017

sidoshi May 20, 2017 •

edited

Loading

gaearon May 20, 2017 •

edited

Loading

		@@ -0,0 +1,187 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

		@@ -0,0 +1,42 @@
		# eslint-restricted-globals [![npm](https://img.shields.io/npm/v/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals) [![npm](https://img.shields.io/npm/l/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals)

		@@ -1,11 +1,11 @@
		# eslint-restricted-globals [![npm](https://img.shields.io/npm/v/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals) [![npm](https://img.shields.io/npm/l/eslint-restricted-globals.svg?style=plastic)](https://www.npmjs.com/package/eslint-restricted-globals)
		# eslint-restricted-globals

Add restricted globals package #2286

Add restricted globals package #2286

Conversation

sidoshi commented May 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidoshi May 20, 2017 • edited Loading

Choose a reason for hiding this comment

gaearon May 20, 2017 • edited Loading

Choose a reason for hiding this comment

sidoshi May 20, 2017 •

edited

Loading

gaearon May 20, 2017 •

edited

Loading