Chore/format and lint #53

Angra974 · 2024-03-25T08:53:35Z

solve #34

chore: format the application with prettier and lint

Description

Briefly explain the purpose of this pull request

Format files and use linters to have a coding convention for everyone to use for readability / code quality and easier debugging time.

What has been done

Added scripts:
"format:check" to check folder apps / docs / and tests folder ( can be changed later if used a different folder for testing the application

"format": will fix the formatting in files from the folders apps / docs / tests only
"format:all" will fix the formatting in files from all the application with exception from .gitignore as prettierignore is not existant.

"format:check": will check and give information about the formatting in files from the folders apps / docs / tests only
"format:checkall" will check and give information about the formatting in files from all the application with exception from .gitignore as prettierignore is not existant.

"lint": will fix ( as much as it can ) files from the folders apps / docs / tests only ( markdown files too )
"lint:all": will fix ( as much as it can ) files from the entire application ( markdown files too )

"lint:check": will give information about problems found in files from the folders apps / docs / tests only
"lint:checkall": will give information about problems found in files from the entire application

Added a pre-commit hook with husky using prettier-quick who will format files according to prettier preference while commiting files. Only staged files will be formatted.

Target files

js,jsx,ts,tsx,mjs,cjs,md

Solves #34

Are there any specific user stories or issues addressed by this PR?

No specific stories
issue resolve #34

As eslint use prettier too, both as been resolved in the same time.
module that need to be updated are done to not have conflict between them for the current build.

...

Testing

Run each command to test the result of each of them.
lint:check =>

lint:checkall

lint:

lint:all =>

format:check =>

format:checkall =>

format =>

format:all =>

Test of precommit hook =>

While adding a component in the apps folder,
we have a new line added "✍️ Fixing up app/components/falseComponent.tsx."
that we didn't have in the precedent commit.
So the formating was done at this time.

For the last commit, I resolved the error shown in the lint:all command.
So, we have now an application that give only warning and no error and only one will be definitively ignored by the linter.

Breaking Changes (optional)

No breaking change

Checklist

I have added unit tests for my changes. (If applicable)
I have manually tested the changes and verified they work as expected.
I have followed the coding style guide.
I have updated the documentation to reflect the changes, including any breaking changes.
I have created regression tests for the feature I am adding.

Reviewer Checklist

Automated test coverage is satisfactory. (If applicable)
PR is fully functional.
PR regressions tested.
[] Documentation regarding the PR has been properly updated.

…se a pre-commit hook too

…ipts path in package.json

render · 2024-03-25T08:53:39Z

Your Render PR Server URL is https://one00hunters-pr-53.onrender.com.

Follow its progress at https://dashboard.render.com/web/srv-co0jn48l6cac73fsaebg.

alcpereira · 2024-03-25T09:18:33Z

.eslintrc.cjs

+    "prettier",
+  ],
+
+  plugins: ["prettier"],


There is already a Prettier configuration (see L28 after your changes), no need to add the plugin. This was the old way to do that before the configuration.

This is following the official docs.

The plugin here is for the md part.
It's use at line 52 "prettier/prettier".
If we take of this like, the tules for md/remark will try to find this plugin.
It's from the repo of eslint-plugin-md i'm using here.
eslint-plugin-md

Can be remove if using another plugin that didn't depend on it or later if we implement remark,
it's what i've done a little later but will be in a separate branch as remark itself will be a new feature and will have it's own way of been handle ( not a perfect one :( ).

alcpereira · 2024-03-25T09:20:05Z

server.ts

@@ -21,7 +21,9 @@ const remixHandler = createRequestHandler({
        viteDevServer.ssrLoadModule(
          "virtual:remix/server-build",
        ) as unknown as Promise<ServerBuild>
-    : await import("./build/server/index.js"),
+    : /* eslint-disable */


If you build the project once and keep build/ folder, you'll never have the linting issue.

I suggest adding a comment to run the build once instead of disabling the line.

ok to delete it.

alcpereira · 2024-03-25T10:57:14Z

package.json

    "rollup": "^4.13.0",
    "storybook": "^8.0.0",
    "tailwindcss": "^3.4.1",
    "tsx": "^4.7.1",
-    "typescript": "^5.1.6",
+    "typescript": "^5.4.0",


Please avoid bumping packages out of the scope of the PR.

It's necessary for not having warning with eslint.
Even if you have typescript 5.1.6 in package.json, it's version 5.4.3 that is used in the application and seems like you didn't see it ^^.
It's the reason we have to use the overrides parameters in package.json sometimes to help up fix the version.
For some people who are using yarn, this can block you from linting and if you use lint with hook, you can push your comment.

It's not the case but in another application they were linting the application in an intermediate state when starting, so it was not starting.

This is a fresh install with typescript 5.1.6 as you can see, pnpm install the version 5.4.3 that is not totally supported for the moment with the version of eslint use.

.eslintignore

package.json

alcpereira · 2024-03-25T11:29:10Z

package.json

@@ -50,7 +56,7 @@
    "@types/react": "^18.2.20",
    "@types/react-dom": "^18.2.7",
    "@typescript-eslint/eslint-plugin": "^6.7.4",
-    "@typescript-eslint/parser": "^6.7.4",
+    "@typescript-eslint/parser": "^7.3.1",


Non-blocker, just a general advice: Always that you bump to a major version, double check the change log to make sure there is no breaking change that might break the project. I checked and this one is okay, but it might have been a breaking one.

You will have this problem with eslint and this version of the parser.
Use with typescript 5.1.6 in package.json and you see again, the application is using v5.4.3.

The path on the lint script in package.json was not good, so eslint was not giving errors / warning because it didn't check something.
And perhaps it will be good to bump @typescript-eslint/eslint-plugin to this version too now that I see it ^^'

.husky/pre-commit

alcpereira · 2024-03-25T11:47:29Z

package.json

+    "lint": "eslint  --cache --fix \"./{app,tests,docs}/**/*.{js,jsx,ts,tsx,mjs,cjs,md}\" ",
+    "lint:all": "eslint  --cache --fix \"./**/*.{js,jsx,ts,tsx,mjs,cjs,md}\"",
+    "lint:check": "eslint  --cache \"./{app,tests,docs}/**/*.{js,jsx,ts,tsx,mjs,cjs,md}\"",
+    "lint:checkall": "eslint  --cache \"./**/*.{js,jsx,ts,tsx,mjs,cjs,md}\"",


I don't see any benefit in having all those commands.

In most projects you'll only find lint, lint:ci (quiet mode), and lint:fix.

Also, defining a list of files path can be quite dangerous, we probably will miss it when refactoring something. I'd keep the lint:checkall (with removing the md extension) as the default for lint.

ok, i'll go for
lint and lint:fix.

package.json

Co-authored-by: alcpereira <[email protected]>

Angra974 added 5 commits March 25, 2024 11:01

chore: added format/lint/check/fix for folders/full application and u…

d1316ad

…se a pre-commit hook too

fix: added rules and plugins for eslint and md files with working scr…

23adea9

…ipts path in package.json

fix: added --cached to eslint to check only changed files too

9216a07

test: added a false component to test pre-commit hook

375520b

test: remove the FalseComponent file from app folder

2244d2e

Angra974 requested review from alcpereira, 20jasper and Pankeking March 25, 2024 08:54

fix: added eslint command to ignore import of build/server/index.js file

b3405fb

alcpereira requested changes Mar 25, 2024

View reviewed changes

Angra974 and others added 3 commits March 25, 2024 17:13

Update package.json

43ec71a

Co-authored-by: alcpereira <[email protected]>

Update package.json

b10aa9d

Co-authored-by: alcpereira <[email protected]>

fix: apply change requested for the PR

3030e65

Angra974 requested a review from alcpereira March 26, 2024 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore/format and lint #53

Chore/format and lint #53

Angra974 commented Mar 25, 2024 •

edited

Loading

render bot commented Mar 25, 2024

alcpereira Mar 25, 2024

Angra974 Mar 25, 2024 •

edited

Loading

alcpereira Mar 25, 2024

Angra974 Mar 25, 2024

alcpereira Mar 25, 2024

Angra974 Mar 25, 2024

alcpereira Mar 25, 2024

Angra974 Mar 25, 2024

alcpereira Mar 25, 2024

Angra974 Mar 25, 2024

Chore/format and lint #53

Are you sure you want to change the base?

Chore/format and lint #53

Conversation

Angra974 commented Mar 25, 2024 • edited Loading

chore: format the application with prettier and lint

Description

Briefly explain the purpose of this pull request

Target files

Are there any specific user stories or issues addressed by this PR?

Testing

Breaking Changes (optional)

Checklist

Reviewer Checklist

render bot commented Mar 25, 2024

Choose a reason for hiding this comment

Angra974 Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Angra974 commented Mar 25, 2024 •

edited

Loading

Angra974 Mar 25, 2024 •

edited

Loading