Skip to content

Conversation

@sidoshi
Copy link
Contributor

@sidoshi sidoshi commented May 20, 2017

Add restricted globals in a separate package.

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use this example:

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add use strict and standard license header.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make it 1.0

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the lock here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's call it confusing-browser-globals.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure how to do this. 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to update this too.

Copy link
Contributor

Choose a reason for hiding this comment

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

and this

Copy link
Contributor

Choose a reason for hiding this comment

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

Two spaces please.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use two spaces for indentation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please use semicolons.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: please use Array.isArray() instead.

@gaearon gaearon added this to the 1.0.2 milestone May 20, 2017
@gaearon gaearon modified the milestones: 1.0.x, 1.0.2 May 20, 2017
Copy link
Contributor

Choose a reason for hiding this comment

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

The title is still old.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, UPDATING.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sidoshi sidoshi May 20, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@gaearon gaearon May 20, 2017

Choose a reason for hiding this comment

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

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);

@gaearon gaearon changed the base branch from master to next January 14, 2018 01:16
@gaearon gaearon modified the milestones: 1.0.x, 2.0.0 Jan 14, 2018
@gaearon gaearon merged commit 6f8e9b8 into facebook:next Jan 14, 2018
Timer pushed a commit that referenced this pull request Jan 14, 2018
* 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
gaearon pushed a commit that referenced this pull request Jan 14, 2018
* 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
gaearon pushed a commit that referenced this pull request Jan 14, 2018
* 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
gaearon pushed a commit that referenced this pull request Jan 14, 2018
* 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
gaearon pushed a commit that referenced this pull request Jan 14, 2018
* 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
Timer pushed a commit to Timer/create-react-app that referenced this pull request Jan 15, 2018
* 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
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
* 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
@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants