GitHub Actions: Implement ESlint

[akibrhast]

Dependency

~#1441~

• #2651
• after the spelling issues are done we can re-review this.
• [ ] Figure out which js files use liquid (and the least liquid in any file)
• [ ] Refactor a js file that uses liquid into two separate files
• [ ] Roll out plan for implementation

Overview

We need to implement a linter for our javascript code to standardize and maintain the code properly.

Action Items

• [ ] Create a list of possible bare minimum esLint settings that we can turn on
  • [ ] During one of the developer meetings, discuss the possible settings with the team and finalize a list of settings to implement
  • [ ] You may reference and use the list from this comment
• [ ] Implement a GitHub action that triggers on pull request or on push to the gh-pages branch
  • [ ] The action should lint javascript files. The recommended tool for linting is Super Linter
  • [ ] Create the action as part of your local fork of the repository
  • [ ] Run the action on js files that should end in a success, and ensure that the action actually succeeds
  • [ ] Run the action on js files that should end in a failure, and ensure that the action fails properly based on the correct settings
  • [ ] Demo your prototype to the team and/or the technical lead to get approval
  • [ ] After approval, create a PR for review
  • [ ] Ensure that the linter does not trigger or break on liquid syntax
• [ ] For merge team: Release dependency on #3230
  • [ ] Update action items in the dependency issue if needed.
  • [ ] If all dependencies in #3230 have been resolved, remove the dependency label, move it to the new issue approval column and add a ready for milestone label.

Resources/Instructions

• A GitHub action for linting the scss files is already implemented, You may use that as reference when implementing this action for Javascript files.
  • lint-scss.yml
  • Issue for implementing scss lint: #1441
  • PR for implementing scss lint: #1957
• Recommended tool for linting: Super Linter
• Super Linter article

Additional resources:

• Events that trigger workflows
• Workflow syntax for GitHub Actions
• GitHub GraphQL

[qiqicodes]

@AmyMendelsohn Can you let us know your progress in regards to this issue? Thank you =]

Progress: Blockers:

[AmyMendelsohn]

@qiqicodes

I ran into some issues getting ESlint running locally.

Unfortunately, I think I don't have enough time to dedicate to this right now. I will unassign this from me.

[averdin2]

• This issue is very closely tied with (https://github.com/hackforla/website/issues/1441). Once I finish up the CSS linter, my idea was to use the same GitHub action with ESLint. See the GitHub action here: https://github.com/marketplace/actions/super-linter

[qiqicodes]

@averdin2 Can you let us know your progress in regards to this issue? Thank you =]

Progress: Blockers:

[averdin2]

Es Lint Getting Started: https://eslint.org/docs/user-guide/getting-started Es Lint Config: https://eslint.org/docs/user-guide/configuring/ Es Lint Rules: https://eslint.org/docs/rules/

[wendywilhelm10]

@sanchece How is the progress on the Es Lint Rules?

[Sihemgourou]

@averdin2 @sanchece Please add update

1. Progress
2. Blockers
3. Availability
4. ETA

[Sihemgourou]

@averdin2 is on vacation. Please tell us when you are back :)

[aliibsin]

unassigned @averdin2 because he's on too many issues

[Sihemgourou]

@averdin2 is going to reach out to @sanchece & @glenflorendo to ask for updates and maybe take over the issue.

[github-actions[bot]]

@sanchece Please add update

1. Progress
2. Blockers
3. Availability
4. ETA

[Sihemgourou]

@averdin2 @sanchece do you have some news about this issue ?

[averdin2]

@glenflorendo I just want to reiterate that you do not need to worry too much about the GitHub action portion of this issue, as I already have a solution for this from working on issue #1441. Just focus on finding usable ESLint rules and I can help coordinate setting up the GitHub action.

[glenflorendo]

I compiled a config of some proposed ESLint rules and a report of the output when I ran these rules on the current codebase. You can view these in the attached txt files.

ESLint Config eslintrc.txt

Output eslint-output.txt

As a suggestion, I think it may be also helpful to invoke ESLint be on the pre-commit (or maybe some other) hook. This helps to assure that every commit made has passed all configured ESLint rules prior to being pushed / merged / etc.

[github-actions[bot]]

@glenflorendo Please add update

1. Progress
2. Blockers
3. Availability
4. ETA

[glenflorendo]

@averdin2 Is there anything else I should do for this issue?

[averdin2]

@glenflorendo If you are confident with your progress on the rules you researched, you can make a eslintrc config file in the root of the project folder. You can then make a pull request draft to merge that file. I am still working on the GitHub action portion of the two linting issues, but I am almost done. When I am done, I will review your proposed rules and implement it with the GitHub action. I am going to put a dependency label on this issue since it is reliant on my progress on the other issue #1441. Until l can review this, I think you should be okay to work on other issues after making the pull request.

[Aveline-art]

@glenflorendo We will remove you from this issue until @averdin2 is done with his ESlint. Please feel free to hop back on when the dependency is completed.

[ExperimentsInHonesty]

has dependency label but no dependency listed at top

[macho-catt]

@macho-catt will rewrite the Overview and Action items to conform to our current standard + explain how it should work with the existing linting action

[github-actions[bot]]

Hi @ronaldpaek, thank you for taking up this issue! Hfla appreciates you :)

Do let fellow developers know about your:- i. Availability: (When are you available to work on the issue/answer questions other programmers might have about your issue?) ii. ETA: (When do you expect this issue to be completed?)

You're awesome!

P.S. - You may not take up another issue until this issue gets merged (or closed). Thanks again :)

[ronaldpaek]

i. Friday - Sunday ii. Sunday, maybe; never implemented a linter before, but I really want to get this done cause I don't like seeing red squigglies on my screen.

[ronaldpaek]

@macho-catt Hi, I'm looking at the previous SCSS linter rules and only selected a few sets of rules. Usually, I opt for the standard config rule set, which is usually recommended and will ensure more consistency. And I reviewed the earlier comments to figure out what rules to apply. I know I usually apply eslint:recommended, and know that will apply pretty good robust setup rules for my js files without enforcing any of the stricter styling rules like semicolons, training commas, etc... Should I set each rule individually and the least amount of rules applied? I considered applying eslint:recommended by itself or with eslint:standard.

I also have another concern/suggestion as well. I know some of the js files I've checked also contain liquid syntax in them, which is why I currently get so many lint warnings. I checked the other day and set the file to be recognized as a liquid file, and the errors went away, but then we lost the IntelliSense, so that's not helpful plus, we want the linter to catch errors. And I believe even if we get eslint to work for JS stuff since the liquid syntax is in the js files, I think it would still throw an error and complain, and we would have to tell es-lint to ignore this block of code manually, like the example shown below.

/* eslint-disable */
{% assign name = 'world' %}
/* eslint-enable */

console.log('Hello, ' + name + '!');

I think this solution would be okay for the first step, and then, we can separate the liquid portion, the data, and the js portion, the logic. potential issues

🙇 @roslynwythe @t-will-gillis

[ronaldpaek]

Hello, I did some research and did a lot of PRs to my repo. I'm not saying it's perfect, but I was just doing some testing, and I first got the super-linter to work for the JS files, then after researching, I saw we can use super linter/slim version which is a little smaller so the GHA/linting finishes faster.

[ronaldpaek]

I then figured out and tested the pre-commit hook feature others mentioned earlier, basically having the linter enforce rule check before making a pull request, which would ensure that every review is formatted correctly and no immediate glaring issues.

It took a bit of figuring out and testing, but I was able to get do pre-commits with JS and CSS

So the pre-commit works, but you can bypass that if you want and commit anyway. Since we already have our linter set up, it isn't a problem. It runs like normal, as if we never had a pre-commit linter going on.

[ronaldpaek]

So I realize all these photos show prettier installed and, yes, prettier, was the last thing I was testing, and they play well with each other. I left just the prettier default setting because I know someone was ticking off every rule set if we know what rules to apply, I think it should be very fast to apply. but seeing as I was committing and pushing/saving files, I don't think the way I have it set up it will try to prettify the whole codebase.

I think some of the rules can be adjusted for prettier as we need. I think we can also adjust it to warn but still be able to make commits as well. Like this head tag thing for liquid/front matter, it doesn't know that that's not an HTML page.

I think this is useful; we can quickly see where inconsistencies are, and should be easy to fix.

I think if a codebase had single and double quotes switching back and forth, it could potentially stress new team members. I know it would stress me out, haha.

And in this example, I clicked a random js file, which already gives me a useful measure. About a variable that isn't defined, I think those things are super helpful on those big files I tend to get overwhelmed. 😆

[ronaldpaek]

Lastly, my last task was to fix the liquid syntax, and actually, that was kind of the reason why I took on this task, but I guess all of this stuff is tied together. I'm not that detailed and organized, so I took a lot of videos and pictures and am reporting my findings.

I could not solve this issue yet, but I am thinking of reorganizing the liquid data, probably in a separate file. I'm just not 100% confident about how the data flows and is used, yet I still need to review the codebase some more.

Oh yes, I actually ended up creating a lot of config files. I wanted to mention I had to init the root folder to node so I could install things on the package.json.

I know I don't think I didn't it when I just did the linter for JS, but when I was looking into the pre-commit stuff since it's a front-end/client side thing, you need a package.json to be in the root, but I'm just reporting my findings I'm sure there is a better way, but at least we have some data, that this is all possible, and it can be done incrementally without breaking everything.

Okay, I am done with my report. 😄 @roslynwythe @t-will-gillis @blulady @ExperimentsInHonesty

[ronaldpaek]

I hope not to sound like a salesman for linting and formatting software. But now I'll post more photos to convince us why we want this.

I think this is a good thing. Large codebases are intimidating, and it's worse if much of the code is not even. Being used, and if it is harder for new people to adopt the style we want, it will ultimately take longer to ramp up.

Clean Code = Happy Coder. It's like studying on a cluttered desk. 😆

I'd be willing to do all transferring/updating and 10000 commits to update every 1 week if you want ;)

[ExperimentsInHonesty]

@ronaldpaek why did you unassign? Where should this issue be moved?

[ExperimentsInHonesty]

Once this issue comes out of the icebox, This issue will need a really good read, and then a decision on what to do next by the dev lead.