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

DO NOT REVIEW: Prettier and research on setup via CL and Plugin #5023

Draft
wants to merge 36 commits into
base: gh-pages
Choose a base branch
from

Conversation

ronaldpaek
Copy link
Member

Fixes #4059

What changes did you make?

  • Added ESlint to GitHub Actions
  • Init root directory to node project
  • Added husky, lintsage for pre-commit
  • Added .lintstagedrc.json, .prettierigonre, .prettierrc.json, .eslintrc.json
  • Added .gitattributes file to normalize EOL settings that were in .vscode
  • scanned all files and applied the changes to them so all files will be consistent
  • made some changes to .vscode settings.json to set prettier as the default

Why did you make the changes (we will use this info to test)?

  • I was unsure, but there was no root package.json file, so I needed the root folder to be a node project to install some packages. I use npm to install prettier, eslint:prettier, husky, and stylelint
  • Initially, I only applied eslint for JS syntax rules, but I noticed similar issues. If we use prettier, we would probably be running eslint, and if we use the eslint:prettier plugin, we should avoid any conflicting issues.
  • I did not see a stylelint and package.json, but since I added a package.json file, I updated and added stylelint as a dependency to control the version along with the other packages.
  • After it worked, I saw another discussion about adding a pre-commit check. That way, if the linter spots any issue, it will warn them to fix the issues first, which should reduce unnecessary work for Pull request reviews.
  • Added the linter to GitHub actions to check js files

image

  • Here is Eslint giving us warning about some syntax errors in JS files.
    And in the terminal is prettier, saying things should be consistent and in double quotes, which will make it very fast/easy to open a file and not really even have to think but have prettier tell you to make to have consistent code with the rules you set

image

  • Here is Husky applying the pre-commit check if it doesn't pass Eslint, it will throw an error. You can always manually bypass and still commit, though, so you are not locked in.

image

  • Eslint tells us we have an undefined variable, which might not throw an error if we didn't have Eslint and could confuse less experienced developers and require more cognitive load to trace through and see what this variable is.

image

  • Ultimately, I took up this issue cause I wanted to fix the warning messages with the liquid syntax in the js file. We must separate the liquid and the js and import the result into the js file. I am still working on figuring out how to do it. Separating the data from the business logic.

@ronaldpaek
Copy link
Member Author

@roslynwythe

@ronaldpaek
Copy link
Member Author

There is another option we could explore https://antfu.me/posts/why-not-prettier. Most people like to split up linters and formatters, but we could have our linter do both if we want. Since a lot of people seem to be apprehensive about being prettier.

@ronaldpaek ronaldpaek marked this pull request as draft July 24, 2023 17:16
@roslynwythe roslynwythe added the Draft Issue is still in the process of being created label Aug 3, 2023
@ronaldpaek ronaldpaek changed the title Prettier and research on setup via CL and Plugin DO NOT REVIEW: Prettier and research on setup via CL and Plugin Oct 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Draft Issue is still in the process of being created
Projects
Status: PR Needs review
Development

Successfully merging this pull request may close these issues.

Considering using Prettier and research on setup via CL and Plugin
2 participants