Update eslint plugins #4
Conversation
|
/cc @Shopify/javascript - new eslint rules. |
|
|
||
| // Async/Await Rules | ||
|
|
||
| // Prefer await to then() for reading Promise values |
There was a problem hiding this comment.
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"); }); }; }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.
There was a problem hiding this comment.
😳 That does look quite scary.
@GoodForOneFare: Should we keep this as a 'warn', or even have it turned 'off'?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
yeah i have strong feels about preferring await with its current browser support. I'd like to hear some of the justification behind this.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
Yup, agreed to turn these two off 👍
There was a problem hiding this comment.
Agreed. I think it's too early for async await at this point.
There was a problem hiding this comment.
Thanks everyone.
The two await rules are off now.
70af7aa
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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!
|
|
||
| // Async/Await Rules | ||
|
|
||
| // Prefer await to then() for reading Promise values |
There was a problem hiding this comment.
Yup, agreed to turn these two off 👍
| @@ -47,6 +49,8 @@ module.exports = { | |||
| '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': 'error', | |||
There was a problem hiding this comment.
IMO this rule is not worth it, I think unset values are fine for boolean props (which would default to falsey values anyways), and I kind of prefer specifying defaults in actual JS rather than defaultProps.
There was a problem hiding this comment.
Fixed with 3b6f088.
| // Avoid using promises inside of callbacks | ||
| 'promise/no-promise-in-callback': 'warn', | ||
| // Avoid calling cb() inside of a then() (use nodeify] instead) | ||
| 'promise/no-callback-in-promise': 'warn', |
There was a problem hiding this comment.
I feel that this rule and the one above are overly strict, I think there are too many valid cases to keep these on.
| // Avoid calling cb() inside of a then() (use nodeify] instead) | ||
| 'promise/no-callback-in-promise': 'warn', | ||
| // Avoid creating new promises outside of utility libs (use pify instead) | ||
| 'promise/avoid-new': 'warn', |
There was a problem hiding this comment.
Definitely turn this one off, we definitely want people to be free to use new Promise whenever they like.
|
GTG once Chris' comments are addressed. Thank you :) |
|
Oh, could you start a |
|
@ismail-syed the omitted rules you are seeing for babel are there because the plugin has not properly declared it's deprecated rules as being deprecated using ESLint's meta object. Even though they have documented these rules as being deprecated in their README, there is no programatic way of To remedy this, I have opened an issues with eslint-plugin-babel, so hopefully when these fixes are made—you should no longer see these omitted rules for the babel plugin. As for the 2 lodash rules, that's got me a little stumped. Are you using the latest version of this plugin? I had a search in the repo and from the change log it looks like these 2 rules were merged into a new Could you please confirm that you have the latest version of |
| // Avoid nested .then() or .catch() statements | ||
| 'promise/no-nesting': 'warn', | ||
| // Avoid using promises inside of callbacks | ||
| 'promise/no-promise-in-callback': 'warn', |
There was a problem hiding this comment.
When working with any of the node API async methods, it's kind of hard to not do this, if you are using any other library that uses promises.
There was a problem hiding this comment.
Fixed with 3b6f088
|
@wagerfield That's what I presumed was the reason behind those Babel rules showing up as omitted. Thanks for the clarification. Regarding The two rules |
|
@ismail-syed I will do some further investigations as to why these lodash rules are being outputted. |
|
Thanks @wagerfield, appreciate it! 💯 |
|
Thanks @wagerfield 👍 |
eslint-indexpackage to help improve scripts to help find omitted lint rulesyarn run rules-statusandyarn run rules-omittedscripts (output shown below)eslint-find-rulespackage and scripteslint-plugin-flowtype,eslint-plugin-mocha,eslint-plugin-promise,eslint-plugin-reactpackageseslint-plugin-reactandeslint-plugin-promiseWhy
lodash/no-single-chainandlodash/prefer-chainare considered 'omitted'.Why babel stuff comes up:
Babel has no CHANGELOG.md, which could be why this isn't getting picked up properly? However, they list it as deprecated on their README.md