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: kickoff repo #1

Merged
merged 5 commits into from
Oct 18, 2024
Merged

feat: kickoff repo #1

merged 5 commits into from
Oct 18, 2024

Conversation

joseacabaneros
Copy link
Member

WIP

package.json Outdated Show resolved Hide resolved
package.json Outdated
},
"devDependencies": {
"eslint": "^9.11.1",
"jiti": "^2.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need jiti ?

Copy link
Member Author

Choose a reason for hiding this comment

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

https://eslint.org/docs/latest/use/configure/configuration-files#typescript-configuration-files
It is to be able to use a TS config file for ESLint eslint.config.ts. With it, the same index config we will export, is also used to lint the project itself.

src/index.ts Outdated Show resolved Hide resolved
.editorconfig Outdated Show resolved Hide resolved
.nvmrc Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@@ -0,0 +1,51 @@
{
"name": "@empathyco/eslint-config",
Copy link
Contributor

Choose a reason for hiding this comment

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

😢 Horrible name
eslint but has prettier config, deps, node-npm version, typescript config...

my suggestion: frontend-config looks better

Copy link
Member Author

Choose a reason for hiding this comment

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

Nono, the main goal for now is using this repo as linter and formatter. The rest of files remains in the repo
As we discussed, for the time being it is not a boilerplate repo

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
Comment on lines 32 to 35
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"lint:inspect": "eslint . --inspect-config",
"prettier": "prettier --write ."
Copy link
Contributor

@davidglezz davidglezz Oct 8, 2024

Choose a reason for hiding this comment

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

almost all Motive fronted project has:

Suggested change
"lint": "eslint .",
"lint:fix": "eslint . --fix",
"lint:inspect": "eslint . --inspect-config",
"prettier": "prettier --write ."
"format": "prettier --write .",
"format-check": "prettier --check .",
"lint": "eslint . --fix",
"lint-check": "eslint ."

why format and not prettier?

We should not stick to a tool name, in the future we could use another tool like Biome.
That is why we have lint and not eslint.

Check vs Fix

There are 2 options,

  1. The short version lint only does checks and a long version lint:fix that fixes the issues.
  2. short version lint fixes and long version lint-check checks.

My proposal is, the short and quick/easy to write version, which is the one we usually use as developer, I would say with the fix included.
That is why I prefer option 2.

Also match with other scripts like typecheck or type-check

"lint:inspect": "eslint . --inspect-config",
It is useful, but only occasionally, I think it will not be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with format instead of prettier.
I also refactored the script as you suggested, but with lint:check or format:check because it is as we use the scripts in every project.

Copy link
Contributor

Choose a reason for hiding this comment

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

"every project" → some

package.json Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Outdated
typescript: {
tsconfigPath: 'tsconfig.json'
},
ignores: ['.loaded_actions', '*.d.ts'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Why '*.d.ts'?? they could also have imports that will be sorted, and other rules also applies to them, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I picked it from the x project and I think it is something custom for the repo than the general eslint config.
Consider it removed.

printWidth: 100,
singleQuote: true,
arrowParens: 'avoid',
trailingComma: 'none',
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be discussed:

Common scenario:

With trailingComma: 'none', :

const x = {
-  a: 3
+  a: 3,
+  b: 4
}

vs trailingComma: 'all', (the default)

const x = {
   a: 3,
+  b: 4,
}

singleQuote: true,
arrowParens: 'avoid',
trailingComma: 'none',
vueIndentScriptAndStyle: true
Copy link
Contributor

Choose a reason for hiding this comment

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

This also,

I vote for false (the default)

In some projects will create a one time commit to format all files. thats all

we win:

  • keep with defaults
  • 2 more chars

cons

  • 1 time commit with changes, after several months it is forgotten.

@AdrianArenal AdrianArenal marked this pull request as ready for review October 18, 2024 10:34
@joseacabaneros joseacabaneros merged commit b1724f1 into main Oct 18, 2024
@joseacabaneros joseacabaneros deleted the kickoff-repo branch October 18, 2024 10:48
This was referenced Oct 18, 2024
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.

4 participants