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

changed the linting for NextJS project using husky #54

Merged
merged 4 commits into from
Oct 25, 2024

Conversation

sank8-2
Copy link
Contributor

@sank8-2 sank8-2 commented Oct 19, 2024

Added husky, lint for nextjs and prettier for code formatting

How to setup

Run npm i from the root directory, all packages and linters gets installed and setup in the local

Then it runs automatically at every commit without extra configuration.

Closes #51

Copy link
Contributor

Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊

@sank8-2
Copy link
Contributor Author

sank8-2 commented Oct 19, 2024

@ahaapple Now you can check it, this one is better and suits a NextJS repo.
Although I haven't checked all the files for linting and formatting

.prettierrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
.prettierignore Outdated Show resolved Hide resolved
.eslintrc.js Outdated Show resolved Hide resolved
@sank8-2
Copy link
Contributor Author

sank8-2 commented Oct 20, 2024

On it

@sank8-2
Copy link
Contributor Author

sank8-2 commented Oct 20, 2024

@ahaapple I have done the necessary changes, please review now

@ahaapple
Copy link
Contributor

  1. There are a lot of duplicate files, which is unreasonable
  2. The contents of the root directory are too messy

@ahaapple
Copy link
Contributor

I think a more elegant solution may be to use GitHub Actions to check each PR to be merged. Developers can use VScode to solve TS checks, code quality checks, and code formatting during the development process. We only need to check once before merging the PR. The benefits of this are:

1 Developers do not need to perform additional operations locally, which is more developer-friendly
2 The code repository will be neater

@sank8-2
Copy link
Contributor Author

sank8-2 commented Oct 20, 2024

Ok, I'll discard the husky and add it to GitHub Actions

@ahaapple
Copy link
Contributor

Run npm run lint
  npm run lint
  shell: /usr/bin/bash -e {0}

> @ lint /home/runner/work/memfree/memfree/frontend
> next lint

You are using Node.js 14.[2](https://github.com/memfreeme/memfree/actions/runs/11513014014/job/32048965546?pr=54#step:5:2)1.3. For Next.js, Node.js version >= v18.17.0 is required.
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @ lint: `next lint`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @ lint script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/runner/.npm/_logs/202[4](https://github.com/memfreeme/memfree/actions/runs/11513014014/job/32048965546?pr=54#step:5:5)-10-2[5](https://github.com/memfreeme/memfree/actions/runs/11513014014/job/32048965546?pr=54#step:5:6)T06_24_49_556Z-debug.log
Error: Process completed with exit code 1.

seems we need to update the node version:

jobs:
  build:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
      - name: Use Node.js 18
        uses: actions/setup-node@v3
        with:
          node-version: '18.x'

@sank8-2
Copy link
Contributor Author

sank8-2 commented Oct 25, 2024

@ahaapple Basically its performing linting and prettier check for the frontend directory
Are any other actions required?

@ahaapple
Copy link
Contributor

@sank8-2 Thanks for your contribution. I think the current actions are enough. When we have enough test cases, we can add the check action of npm run test later.

@ahaapple ahaapple merged commit 32b3a08 into memfreeme:main Oct 25, 2024
2 checks passed
Copy link
Contributor

🎉 Your pull request has been successfully merged! 🎉 Thank you for your valuable contribution to our project. Your efforts are greatly appreciated. Feel free to reach out if you have any more contributions or if there's anything else we can assist you with. Keep up the fantastic work! 🚀

@sank8-2
Copy link
Contributor Author

sank8-2 commented Oct 25, 2024

@ahaapple Ok
Thank you

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.

Better pre-commit config for nextjs project
2 participants