vigetlabs / blendid

A delicious blend of gulp tasks combined into a configurable asset pipeline and static site builder
MIT License
4.97k stars 683 forks source link

add css/js linters #517

Open benjtinsley opened 6 years ago

benjtinsley commented 6 years ago

i discussed this with @ten1seven recently and thought it would be a good feature for the next release. obviously configurable but the default setting is viget standard. @gregkohn @davist11 @nhunzaker do you have any opinions here?

davist11 commented 6 years ago

Is CSS/JS linting something we plan to use on every project? Genuinely asking, I've never used a CSS linter and only used a JS linter a few times. We definitely want to slim Blendid down, so maybe having an easy way to add it, but not part of the Blendid could be an option?

benjtinsley commented 6 years ago

@davist11 yea i think it would be used on every project, viget-wise. to me this will prevent a lot of miniscule errors that are normally caught in PRs (non alphabetized CSS properties, inconsistent semicolons in JS). i think the key here, if we do go forward with this, is to make sure the blendid linter standard stays the same as the viget linter standard if we do set it by default. this will allow a much easier project setup and will (hopefully) allow PR comments to focus on what code does rather than silly formatting comments.

nhunzaker commented 6 years ago

I've never used an automated CSS linter, however we use a JavaScript linter on every client-side app project. It catches too many things early that always result in runtime errors.

We use https://github.com/MoOx/eslint-loader for that, so that the lint warnings show up in the webpack compiler output. This also means that webpack fails if certain rules are set to "error". This is really nice when developing because it catches mistakes early. An undefined variable or bad import is always an error. For these cases, eslint gives us much better feedback, often seconds/minutes earlier.

Beyond that, we don't use eslint to enforce many code formatting rules. At the very most we warn, and I've thought about even removing that. We just use Prettier with a CI enforced check.

I think it is essential to avoid interrupting the local development workflow. Forgetting white space or a semicolon should never fail the build. Here are our rules:

https://gist.github.com/nhunzaker/757c4e95009a9eb5d77ae5f2266c11de

In many cases, we throw errors on CI only for things like forgotten debuggers, using .only to refine test output. Basically things we tend to accidentally commit into version control that are 99.9% oversight mistakes.

Beyond that, I think it's important that the command line, editor, and continuous-integration experience is consistent. There shouldn't be surprises when a build fails for linting issues.

ten1seven commented 6 years ago

I mentioned this to @benjtinsley because it's something I think we should start doing across all projects to enforce some basic standards. Adding linters to Blendid's core (they're part of the install and run with the build process) along with configs that meet our standards would achieve that pretty easily.

illycz commented 6 years ago

When you plan to release a fifth version?

benjtinsley commented 6 years ago

@illycz we dont have a date yet, but probably sometime before spring

armpogart commented 6 years ago

@benjtinsley From my own experience I would recommend stylelint as css linter (it can even lint SASS/SCSS with additional plugin) and eslint as javascript linter which has pretty good standard and recommended configurations out of the box.

olets commented 5 years ago

See also #327 (and PR #345) and #425