Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add lintfix and prettier scripts #829

Merged
merged 3 commits into from
Nov 18, 2021

Conversation

scscgit
Copy link
Contributor

@scscgit scscgit commented Jul 18, 2021

  • Add a lint:prettier script to provide users with a recommended way to enforce a unified code style in the project, which is less accessible if they have to research the correct usage, or even access it via npx. The intention should be to provide a configuration where linters don't conflict with each other. Also included this in the lint script by default to enforce best practices (but anyone is free to remove it). I can't comprehend why new users should be forced to research this on their own; we need a reference solution as a source of truth for example if we want to setup IDE extensions and make sure that they converge on the same code style (definitely not trivial with all the options in VSCode like Vetur, Prettier, Eslint, built-in...).
  • Add a lintfix script to provide a quick way of fixing any issues in the entire project, especially before they make a commit. Prettier includes --list-different to display only changed files (unified with other linters) instead of every single file in the project.
  • create-nuxt-app: run lintfix at the end instead of hard-coding all of the scripts individually; just like what the user is supposed to do on a regular basis.
  • Extend lint-staged in package.json to include prettier by default, where it requires --ignore-unknown Implement a new --ignore-unknown argument for CLI prettier/prettier#8136, as you can reproduce the issue by trying to commit an unstyled file that's referenced in the .prettierignore. Additionally use --cache for eslint as a recommended performance improvement, which is already assumed as our current .gitignore includes the .eslintcache.
  • Reordered scripts to have linters near each others, and to keep the prepare at the end. (Or could we move it to the top?)
  • Add .ts to supported eslint types. I'd personally prefer keeping it there even if the project is JS-only, but for the sake of consistency I only added it for TS projects.
  • Add .scss, .sass, and .html to supported stylelint types. Fixing CSS <style> tags inside HTML definitely sounds like an expected default behavior to me. This list may not be exhaustive, so please make sure it's kept up-to-date with any popular use-cases.
  • Modify lintStaged conditions to include it even without eslint. Previously this meant that together with husky they weren't configured if the user has only chosen commitlint, which I believe is a bug. Now it installs even for prettier, but I also allowed it to be installed as an empty block if chosen alone, because the effects of that selection don't make any sense otherwise; who are we to judge if someone wants to lint something else on staging?

_
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of lintfix results thanks to prettier; I think we should integrate it to the template.

prettier: '<%= pmRun %> lint:prettier'
}
const lintfixScripts = {
eslint: "<%= pmRun %> lint:js <%= pm === 'npm' ? '-- ' : '' %>--fix",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used EJS templates because there already was <%= pmRun %>, but I don't see a reason not to `use ${variables} instead`; especially as any incorrect templates in this JS cause a compilation error, e.g. if you escape \' within " instead of replacing ' by " and using ' inside. Also the whitespace-trim didn't seem to be very useful here.

delete pkg.devDependencies.husky
delete pkg.scripts.prepare
} else {
// Move prepare to make it the last script
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of moving, we could also create it here.

@@ -2365,12 +2374,17 @@ Generated by [AVA](https://avajs.dev).
'core-js': '^3.15.1',
nuxt: '^2.15.7',
},
devDependencies: {},
Copy link
Contributor Author

@scscgit scscgit Jul 18, 2021

Choose a reason for hiding this comment

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

Here the lint-staged was selected alone, which previously didn't install it.

@scscgit
Copy link
Contributor Author

scscgit commented Jul 28, 2021

I'd also suggest removing the .prettierrc and instead moving its content to package.json (as supported by cosmiconfig) in order to reduce the unnecessary amount of files in the root directory. Most users won't ever touch the defaults, and there doesn't seem to be any advantage in a separate file anyway (e.g. having JS available):

"prettier": {
    "semi": false,
    "singleQuote": true
  },

@scscgit
Copy link
Contributor Author

scscgit commented Sep 6, 2021

Reordered prettier to the beginning of lintfix to fix #827.

@scscgit scscgit changed the title feat: lintfix and prettier scripts, fix lint-staged coverage feat: lintfix and prettier scripts, fix lint-staged coverage, fix #827 by reordering prettier in lintfix to avoid eslint conflicts Nov 3, 2021
@scscgit scscgit changed the title feat: lintfix and prettier scripts, fix lint-staged coverage, fix #827 by reordering prettier in lintfix to avoid eslint conflicts feat: lintfix and prettier scripts, fix lint-staged coverage, fix #827 to avoid eslint conflicts by reordering prettier in lintfix Nov 3, 2021
@clarkdo clarkdo changed the title feat: lintfix and prettier scripts, fix lint-staged coverage, fix #827 to avoid eslint conflicts by reordering prettier in lintfix feat: add lintfix and prettier scripts Nov 18, 2021
@clarkdo clarkdo merged commit f3e61cd into nuxt:master Nov 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants