Skip to content
This repository has been archived by the owner on Aug 18, 2020. It is now read-only.

Update eslint plugins #4

Merged
merged 10 commits into from
Jan 17, 2017
Merged

Update eslint plugins #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7111ff6
Adding new eslint-index package :package:
ismail-syed Jan 13, 2017
4e5de59
Update package versions :package:
ismail-syed Jan 13, 2017
bc30696
Add new rules for eslint-plugin-react
ismail-syed Jan 13, 2017
4cc649b
Add new rules for eslint-plugin-promise
ismail-syed Jan 13, 2017
6e26878
Fix eslint-index package version
ismail-syed Jan 16, 2017
5fbefbc
Remove eslint-find-rules package and script
ismail-syed Jan 16, 2017
70af7aa
Update eslint-plugin-promise rules
ismail-syed Jan 16, 2017
8daa0d0
Enable ignorePureComponents flag for react/prefer-stateless-function …
ismail-syed Jan 17, 2017
40de604
Update eslint-plugin-lodash package to 2.3.x and add new rule
ismail-syed Jan 17, 2017
3b6f088
Updated rules
ismail-syed Jan 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lib/config/rules/lodash.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ module.exports = {
'lodash/consistent-compose': ['warn', 'flow'],
// Prefer identity shorthand syntax
'lodash/identity-shorthand': ['warn', 'never'],
// Prefer a specific import scope (e.g. lodash/map vs lodash)
'lodash/import-scope': ['error', 'method'],
// Prefer matches property shorthand syntax
'lodash/matches-prop-shorthand': ['warn', 'never'],
// Prefer matches shorthand syntax
Expand Down
20 changes: 20 additions & 0 deletions lib/config/rules/promise.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,33 @@
// see https://github.com/xjamundx/eslint-plugin-promise

module.exports = {
// Promise Rules

'promise/always-catch': 'off',
// Ensure that each time a then() is applied to a promise, a catch() is applied as well. Exceptions are made if you are returning that promise.
'promise/catch-or-return': 'warn',
// Avoid wrapping values in Promise.resolve or Promise.reject when not needed
'promise/no-return-wrap': 'error',
// Ensure that inside a then() you make sure to return a new promise or value.
'promise/always-return': 'error',
// Enforce standard parameter names for Promise constructors.
'promise/param-names': 'warn',
// Ensure that Promise is included fresh in each file instead of relying on the existence of a native promise implementation.
'promise/no-native': 'off',
// Avoid nested .then() or .catch() statements
'promise/no-nesting': 'warn',
// Avoid using promises inside of callbacks
'promise/no-promise-in-callback': 'off',
// Avoid calling cb() inside of a then() (use nodeify instead)
'promise/no-callback-in-promise': 'off',
// Avoid creating new promises outside of utility libs (use pify instead)
'promise/avoid-new': 'off',


// Async/Await Rules

// Prefer await to then() for reading Promise values
Copy link

Choose a reason for hiding this comment

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

Are we using the babel compiler for this or something else? Their implementation is not what I'd call light weight.

ex:

async function doTheThing() {
  const whatever = await fetch('google.com');
  
  return whatever;
}

transforms to:

var doTheThing = function () {
  var _ref = _asyncToGenerator(regeneratorRuntime.mark(function _callee() {
    var whatever;
    return regeneratorRuntime.wrap(function _callee$(_context) {
      while (1) {
        switch (_context.prev = _context.next) {
          case 0:
            _context.next = 2;
            return fetch('google.com');

          case 2:
            whatever = _context.sent;
            return _context.abrupt('return', whatever);

          case 4:
          case 'end':
            return _context.stop();
        }
      }
    }, _callee, this);
  }));

  return function doTheThing() {
    return _ref.apply(this, arguments);
  };
}();

function _asyncToGenerator(fn) { return function () { var gen = fn.apply(this, arguments); return new Promise(function (resolve, reject) { function step(key, arg) { try { var info = gen[key](arg); var value = info.value; } catch (error) { reject(error); return; } if (info.done) { resolve(value); } else { return Promise.resolve(value).then(function (value) { step("next", value); }, function (err) { step("throw", err); }); } } return step("next"); }); }; }

source

It also creates a blocking loop with the while(1) which is kinda scary. It does look like there's a defined exit condition, but it's hard to say from the generated code how many iterations it'll block on.

Copy link
Contributor Author

@ismail-syed ismail-syed Jan 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

😳 That does look quite scary.
@GoodForOneFare: Should we keep this as a 'warn', or even have it turned 'off'?

Copy link

Choose a reason for hiding this comment

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

Given the blocking behaviour, is it even advantageous to encourage the use of await? I feel like it produce a specific behaviour that may be desirable only in specific circumstances (where blocking IO is required)

Copy link

Choose a reason for hiding this comment

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

yeah i have strong feels about preferring await with its current browser support. I'd like to hear some of the justification behind this.

Copy link
Member

Choose a reason for hiding this comment

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

@ismail-syed is just getting started on FED, so these are just his best guesses. Agree that this should be off until browser support is better.

Copy link
Member

Choose a reason for hiding this comment

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

Yup, agreed to turn these two off 👍

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. I think it's too early for async await at this point.

Copy link
Contributor Author

@ismail-syed ismail-syed Jan 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks everyone.
The two await rules are off now.
70af7aa

Copy link

@hzoo hzoo Jan 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, not using async also would help with removing regeneratorRuntime. Are y'all using babel-polyfill as well?

Alternatives: you can try https://github.com/babel/kneden although needs work since it's not stable yet, https://github.com/MatAtBread/fast-async, or use http://babeljs.io/docs/plugins/transform-async-to-generator if you support browsers that have generators natively. Would recommend https://github.com/babel/babel-preset-env/ for that as well.

Sorry for the random comment haha

Copy link
Member

Choose a reason for hiding this comment

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

@hzoo All of our projects support at least IE11, so we're still stuck without native generators, and I'd rather way for async-await until we can do so based on native generators, rather than any kind of runtime. We're excited to start using babel-preset-env (Shopify/babel-preset-shopify#4), though. Thanks very much for all your hard work on the Babel project!

'promise/prefer-await-to-then': 'off',
// Prefer async/await to the callback pattern
'promise/prefer-await-to-callbacks': 'off',
};
6 changes: 5 additions & 1 deletion lib/config/rules/react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ module.exports = {
'react/forbid-component-props': 'off',
// Forbid certain propTypes
'react/forbid-prop-types': ['error', {forbid: ['any', 'array']}],
// Prevent using Array index in key prop
'react/no-array-index-key': 'error',
// Prevent passing children as props
'react/no-children-prop': 'warn',
// Prevent usage of dangerous JSX properties
Expand Down Expand Up @@ -42,11 +44,13 @@ module.exports = {
// Enforce ES5 or ES6 class for React Components
'react/prefer-es6-class': 'error',
// Enforce stateless React Components to be written as a pure function
'react/prefer-stateless-function': 'warn',
'react/prefer-stateless-function': ['warn', {ignorePureComponents: true}],
// Prevent missing props validation in a React component definition
'react/prop-types': 'warn',
// Prevent missing React when using JSX
'react/react-in-jsx-scope': 'error',
// Enforce a defaultProps definition for every prop that is not a required prop
'react/require-default-props': 'off',
// Enforce React components to have a shouldComponentUpdate method
'react/require-optimization': 'off',
// Enforce ES5 or ES6 class for returning value in render function
Expand Down
17 changes: 9 additions & 8 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
"scripts": {
"check": "yarn run lint && yarn test",
"deploy": "npm publish",
"check-rules": "eslint-find-rules --unused lib/config/all.js",
"rules-status": "eslint-index lib/config/all.js --format table",
"rules-omitted": "eslint-index lib/config/all.js --status omitted",
"lint": "eslint . --max-warnings 0",
"test": "NODE_PATH=$NODE_PATH:./transforms:./tests mocha 'tests/**/*.js' --reporter spec --compilers js:babel-core/register",
"test:watch": "yarn test -- --watch --reporter min",
Expand All @@ -40,9 +41,9 @@
"babel-cli": "6.x",
"babel-core": "6.x",
"babel-preset-shopify": "15.0.1",
"eslint-find-rules": "1.x",
"eslint-plugin-shopify": "file:.",
"eslint": "3.10.x",
"eslint-index": "1.2.x",
"eslint-plugin-shopify": "file:.",
"isparta": "4.x",
"istanbul": "0.4.4",
"mocha": "3.x"
Expand All @@ -55,14 +56,14 @@
"eslint-plugin-ava": "4.0.x",
"eslint-plugin-babel": "4.0.x",
"eslint-plugin-chai-expect": "1.1.x",
"eslint-plugin-flowtype": "2.28.x",
"eslint-plugin-flowtype": "2.29.x",
"eslint-plugin-import": "2.2.x",
"eslint-plugin-jsx-a11y": "3.0.x",
"eslint-plugin-lodash": "2.2.x",
"eslint-plugin-mocha": "4.7.x",
"eslint-plugin-lodash": "2.3.x",
"eslint-plugin-mocha": "4.8.x",
"eslint-plugin-node": "3.0.x",
"eslint-plugin-promise": "3.0.x",
"eslint-plugin-react": "6.7.x",
"eslint-plugin-promise": "3.4.x",
"eslint-plugin-react": "6.9.x",
"eslint-plugin-sort-class-members": "1.1.x",
"merge": "1.x"
}
Expand Down
Loading