fix: production builds should fail when lint errors are reported

[zebateira]

By using the option failOnError we can make the build fail every time we have a lint issue.

Closes #71

[zebateira]

3f22b4f was necessary because of: travis-ci/travis-ci#1066

[terichadbourne]

@zebateira I like this suggestion, but I assume we may need to go back through and lint every file consistently before merging this one.

We've not each been using identical linters on our own machines during development and have taken a pretty practical approach. Some of us have likely been using backticks or double quotes to avoid escaping apostrophes in text, for example, not just for interpolation. The quotation issue will be prevalent enough for tutorial authors that if we're going to enforce consistency, we should add a note on it to the Developing Tutorials guide, keeping in mind some of our authors creating multiple-choice or text-based lessons won't be particularly familiar with JavaScript.

I noticed that @olizilla's original proposals in #71 were to:

• add npm run lint to the travis config so we get feedback on PRs (to which @mikeal added that it's a little more involved than that b/c our current travis config is for gh-pages deploys so it doesn't run on every PR, only on merges to master)
• figure out why the vue-cli-service lint reports no lint errors when mixed quote styles are used.

But it appears you're taking a different approach with editing vue.config.js. Since I'm not very familiar with our build tools, I'd love it if you could help me understand why you've taken a different approach and at what stage it will be flagged to the PR author (for example, will it show up as an error in the Travis build before we click to merge, even though we're not merging directly to master?)

Given that this isn't my area of expertise, I'd really appreciate a review from @mikeal or @olizilla before we merge.

[zebateira]

@terichadbourne no worries!

but I assume we may need to go back through and lint every file consistently before merging this one.

Fortunately no. the linter on the travis build (and locally) runs on every file, and the only lint error reported is the one I fixed here. So all is good. Some inconsistencies will arise from the fact that there are some PRs open that do not have this new build config set, but all is good since in future PRs they will be fixed if necessary.

We've not each been using identical linters on our own machines during development and have taken a pretty practical approach. Some of us have likely been using backticks or double quotes to avoid escaping apostrophes in text, for example, not just for interpolation. The quotation issue will be prevalent enough for tutorial authors that if we're going to enforce consistency, we should add a note on it to the Developing Tutorials guide, keeping in mind some of our authors creating multiple-choice or text-based lessons won't be particularly familiar with JavaScript.

I see, this is a good point. I agree with adding this detail to the Developing Tutorials guide - in the cases where the authors are not well versed in Javascript, would it be feasible for us to pinch in and fix the lint issues? I mean, will they be a small number or will they be the majority?

But it appears you're taking a different approach with editing vue.config.js. Since I'm not very familiar with our build tools, I'd love it if you could help me understand why you've taken a different approach and at what stage it will be flagged to the PR author (for example, will it show up as an error in the Travis build before we click to merge, even though we're not merging directly to master?)

Of course!

• add npm run lint to the travis config

We could've added it to the config but npm run build already lints the project, so linting is already happening (in fact, if you go through past builds you'll see some lint issues being reported).

• figure out why the vue-cli-service lint reports no lint errors when mixed quote styles are used.

Actually the reason for this is because the webpack loader eslint-loader will not failOnError, meaning the lint errors will not cause an error while linting. So we set this option in the vue config (80df911) and now the build fails... but not for the right reasons - this build ran right after I made the lint fail when a linting issue occurs. If you noticed the tests are failing - this means that the tests command is running after the build command failed.

The travis build is not crashing when one of the scripts fails: travis-ci/travis-ci#1066 which is a default behavior of travis that I never had thought of 🤔to "fix" this, we can add the command set -e to make the travis build fail fast as soon as the build command fails.

at what stage it will be flagged to the PR author (for example, will it show up as an error in the Travis build before we click to merge, even though we're not merging directly to master?)

Yes, that is correct: it will show on the PR checks bellow the discussion like so:

Let me know if this is clear.

[zebateira]

Actually, I'm now seeing that set -e is not a good workaround since it can make the build fail on success... Will revert back - having the tests failing is not so bad, it just makes the build 2 minutes slower.

[terichadbourne]

Thanks for the detailed explanation @zebateira!

I agree with adding this detail to the Developing Tutorials guide - in the cases where the authors are not well versed in Javascript, would it be feasible for us to pinch in and fix the lint issues? I mean, will they be a small number or will they be the majority?

For those building coding tutorials, we shouldn't have any trouble, but I'm hoping to play up the angle that non-coders can contribute text-based and multiple-choice lessons using our boilerplates. The concept of escaping an apostrophe is something I'd love for them to never have to learn. You can see I already have both an apostrophe in double quotes and an escaped ones in single quotes elsewhere in the mult choice boilerplate. 😂 So we should probably add notes on this in two spots: the developing a tutorial guide (maybe the troubleshooting section) and in the boilerplates themselves (as comments), at least the multiple choice one. Looks like the standard one only requires them to insert the lesson number there's not too much that should go awry there.

Yes, we can always jump in right before a merge and help out ourselves (we'll need to fix the tutorial lists at that stage regardless to determine where and in what order the new submission displays), but:

• When we edit each other's code we have direct access to fix it, but when folks with different levels of access start submitting from forks it will be much harder to make edits (unless there's a trick I haven't learned yet)
• If their own linters on their laptops won't care and only Travis will, it's okay for us to deal with it, but if they may be using a program with a built-in linter that might catch the same errors, it would be kind to teach them in advance what to do.

While we await a review on the Travis piece from @olizilla or @mikeal, would you mind proposing edits to the boilerplates (I'd recommend checking them all for consistent formatting just in case) and the Developing tutorials guide to go along with the new linting enforcement?

[zebateira]

@terichadbourne I see. I think I have a proper solution for this linting issue:

We can simply add a pre-commit hook that runs npm run lint and makes sure that the code is autofixed. Any other errors that are reported will disallow a user from pushing with linting errors (worst case scenario since autofix should take care of most cases).

This should mitigate most of the issues. If someone has a hard time submitting something we can help them and revisit this issue if we find it appropriate.

So I'll go ahead and work on the following if you are ok with it:

• Go through the boilerplates and review the quotes and double quotes styles
• Add pre-commit to automatically fix linting issues before any contributor pushes for a PR

[terichadbourne]

@zebateira I'm okay with this plan if we're positive that auto-linting won't break anything (I have an auto-lint, then auto-save thing going in one of my editors right now related to quotes that has repeatedly messed up classes and made things un-renderable by making the wrong assumptions about what quotes belong where).

[zebateira]

I'm okay with this plan if we're positive that auto-linting won't break anything

The auto-linking on the pre-commit hook will be editor independent since it will run using the projects configs always - so it will be consistent across contributors (no matter what editor they use).

[zebateira]

going to skip adding a pre-commit hook since it seems we already have other git hooks in place (some dependency is installing them).

[zebateira]

@terichadbourne ready to go - I added a note in the troubleshooting section of the developing tutorials guide about linting the project.

[terichadbourne]

@zebateira Your comments highlighted the fact that we didn't have any instructions for submitting a PR, so I've reworked the guide a bit to include that. Mind proofing to see if it looks good to you?

[zebateira]

@terichadbourne LGTM 🙌