fastify / point-of-view

Template rendering plugin for Fastify
MIT License
344 stars 87 forks source link

Add eslint and code format configuration #277

Closed danielkhan closed 2 years ago

danielkhan commented 2 years ago

Added eslint and copied all config files for code style over from fastify to make contributing easier.

Checklist

danielkhan commented 2 years ago

If we install this packages, I think we should at least use it in command instead of IDE only.

I tried but looking closer, it seems as if the eslint command is only used to validate typescript while eslint by configuration extends standard. By running standard on npm test the same rules the IDE uses through eslint are applied.

danielkhan commented 2 years ago

@climba03003 are we good to go? Don't want to gloss over your objection.

danielkhan commented 2 years ago

So, I only use the command before merging but not rely on the auto linting feature of IDE. And that is the reason why I think if you add it, you need to use it. As it do nothing in my environment, just increase the installation time.

Understood. Technically it is used as most IDEs will look inside the current workspace when trying to find eslint. But the fact that it does now work on my machine, is of course not too meaningful. Yet sticking to the same ruleset as fastify makes sense in my opinion.

danielkhan commented 2 years ago

I would like to ask you for one more test on your PC: starting from master, could you add a simple .eslintrc like this:

I've done that, cleaned node_modules, reinstalled without these deps. Turns out that Standard itself has the dependencies already, so I omitted them in my package.json now.

$ npm ls eslint
point-of-view@4.15.3 /Users/dkhan/CodeKitchen/point-of-view
└─┬ standard@16.0.4
  ├─┬ eslint-config-standard-jsx@10.0.0
  │ └── eslint@7.18.0 deduped
  ├─┬ eslint-config-standard@16.0.3
  │ └── eslint@7.18.0 deduped
  ├─┬ eslint-plugin-import@2.24.2
  │ └── eslint@7.18.0 deduped
  ├─┬ eslint-plugin-node@11.1.0
  │ ├─┬ eslint-plugin-es@3.0.1
  │ │ └── eslint@7.18.0 deduped
  │ └── eslint@7.18.0 deduped
  ├─┬ eslint-plugin-promise@5.1.1
  │ └── eslint@7.18.0 deduped
  ├─┬ eslint-plugin-react@7.25.3
  │ └── eslint@7.18.0 deduped
  └── eslint@7.18.0
mcollina commented 2 years ago

I think I landed the other PR first :( Can you rebase this?

danielkhan commented 2 years ago

I think I landed the other PR first :( Can you rebase this?

I was able to merge this.

climba03003 commented 2 years ago

I think I landed the other PR first :( Can you rebase this?

I was able to merge this.

Can you remove the eslint related plugin inside this PR. As they are added in another PR.

danielkhan commented 2 years ago

Can you remove the eslint related plugin inside this PR. As they are added in another PR.

Correct. Done.