Revisit rules prior to 2.0.0 release

[kieckhafer]

After auditing all existing eslint warnings, we will be releasing v.2.0.0 of this config. We should spend some time revisiting the existing rules and adding any new rules we see fit prior to this release.

Please use this ticket for any discussion about what to remove / add / update.

[rosshadden]

I started a list a couple weeks ago as I added our linting rules to a codebase that did not previously have them. In my prime I used to get fired up about linting rules but nowdays I only have problems with the weird pointy ones that tell me I can't do things that every other kid on the block does. Mom and Dad, please.

• prefer-destructuring: You should be able to destructure, sure, but you should not be forced to. It is currently an error in our config, and I would like the rule to be disabled.
• space-before-function-paren: Our current config requires different rules based on the context. I don't care what we do but it should be consistent. Always no space function() {} is pretty conventional, but if somebody pushes hard for function () {} I won't glare at them. Things like this fall into "who cares as long as it's consistent and --fixable".
• one-var: I don't understand the pedantry of requiring variable declarations on multiple lines. I don't really care, but I'd rather this not be enforced. It's common to declare several variables at once in a certain scope like outside a loop and then set them later inside another scope.

I care even less about these but don't really understand why they are enforced:

• promise/always-return
• promise/catch-or-return

[kieckhafer]

@zenweasel @spencern @aldeed

Are there any new rules that you would like enforced, or older rules that should be removed, in this 2.0.0 release: https://github.com/reactioncommerce/reaction-eslint-config/pull/17

What about the rules @rosshadden mentioned above?

@aldeed you mentioned a few here, still contemplating those? https://github.com/reactioncommerce/reaction-eslint-config/issues/5

[aldeed]

• Turn off "prefer destructuring": Fine with me either way
• Function () spacing: The way it is is how I was taught years ago: no space when named, yes space when anonymous because function is a keyword. (From Crockford's original guidelines I believe.) So I'm partial to that. But I think it's somewhat a moot point because we also have eslint rule that requires all anonymous functions be arrow functions, so function () { should never appear anywhere. I think in a perfect world, I'd actually disallow anonymous functions entirely because names are helpful in the stack trace.
• one-var: I grew to like this over time, especially when I was helping teach people who were new to JS because all the other ways you can declare and format variables seem to confuse people. It's more characters, but nowadays I've practically forgotten that there's any other way to declare them. (Similar to how we always use semicolons at the end of lines even though it's almost never necessary.) It wouldn't bother me if we want to remove the rule, but I'd still probably do it that way.
• Promise rules: I'm not sure about always-return but catch-or-return helps avoid errors being swallowed so it's very important. I can't tell you how many hours I've lost debugging issues that were due to errors being thrown in async functions that didn't have a catch anywhere in the chain.
• Yes, I would still like to see the Node plugin added. We can use the recommended config for that and disable any that we don't like, but I think in particular the ones that ensure we are not using deprecated APIs or APIs that are unavailable in the depended version of Node are useful.

I'd also update to latest of all the plugin packages if you haven't already @kieckhafer

[brent-hoover]

Well, just a note that things like prefer-destructuring aren't for people like @rosshadden, they are for less experienced people who just don't do it, and in general when you can, you should. You can always disable it for a particular line if you have a good reason to do it and a comment why you did is probably good learning for everybody and without that we don't know if you intended to or just forgot.

That and catch-and-return are good guardrails for people imho. Experts can break the rules when needed.

[kieckhafer]

So it seems like we should keep all the rules that were mentioned to be seen more as helpful rules for beginners as opposed to annoying rules for more experienced developers.

[rc-publisher]

:tada: This issue has been resolved in version 1.10.0 :tada:

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot :package::rocket: