Switching from `jshint` to `eslint`

PopGoesTheWza commented 4 years ago

Project currently relies on jshint for linting. eslint seems to offer more rules, configuration options, and support for ESnext (and TypeScript)

Integration within editor (at least with Visual Studio Code) is also superior with better identification of which rule triggers, optional 'auto fix' options.

Last, eslint integrates with Prettier for a single "fix lint errors and format code" action.

koresar commented 4 years ago

Yeah. I was trying to migrate to eslint and Prettier the other day but lack of time... I'd love to see this repo migrated to node.js 8, eslint and Prettier. :) Go ahead!

PopGoesTheWza commented 4 years ago

We'll need a dedicated branch where I'll put basic setup and where you'll tune rules per your liking. If you already have some identified preset eslint rules, let it be known. Else I'll go with AirBnB presets.

koresar commented 4 years ago

I have my likings. I call them "As less configs as possible". Meaning that:

Default (built-in) eslint rules - "eslint:recommended"
Default Prettier formatting.
Prefer keeping all configs inside the package.json to minimise the number of config files in the project.

I've been with AirBnB presets for 3 years (since they launched them). Long story short - I had to stop using them because they change rules from time to time and I had no capacity to keep up to date with them.

PopGoesTheWza commented 4 years ago

sigh

I personally prefer to separate each component config in its dedicated files in json format if it allows comments, else javascript (yaml as last resort)

long package.json where everything is configured are a pain (imho) to maintain.

koresar commented 4 years ago

Long pacakge.json is pain to maintain. Agree. Many configuration is difficult to maintain in general. Thus, "as less config as possible" and "as less config files as possible" is the best combo. (I'm doing node.js only development for 6 years. Learned it the hard way!)

Let's do the following:

Have config files per each tool as you prefer.
Have as much configuration defaults as I prefer.

If you still strongly do not agree with me - feel free to do whatever you like. :) I'm will accept your choices since you're working on it.

PopGoesTheWza commented 4 years ago

For initial setup, in dedicated branch, we'll experiment with distinct config files which should make things easier to track and change until we reach a point where merging make sense. At this point we can opt to keep files or move them into package.json

PopGoesTheWza commented 4 years ago

@koresar I created an eslint branch with an initial set of rules.

eslint-config-airbnb-base
eslint-config-prettier + eslint-plugin-prettier (for Prettier to act as an eslint rule)
eslint-plugin-import (for import/export related rules)
eslint-plugin-jest (Jest related rules)
eslint-plugin-node (NodeJS related rules)

current npm run lint is bound to fail until the .js go through various --fix cycle

koresar commented 4 years ago

We do not have bundlers in this repo (no webpack, no rollup). Node.js 8,10,12 does not support import/export by default. Thus we do not have import/export syntax here. Why do we need eslint-plugin-import?

This repo code does not do any I/O and does not use node.js API. Thus eslint-plugin-node sounds useless too.

Also, knowing what the eslint-plugin-jest was created for I can see it mostly useless too.

The @stamp/* things are a pure JavaScript code to create JavaScript objects from JavaScript objects. :) Like lodash does.

Some history.

We had hard times having babel, airbnb linting, bundler in this repo. They do work, however, few months later they broke everything. We had issues after issues related to various maintenance bugs.

Please, think twice before doing all that work on your free time. It might harm the project longer term. But, as always, feel free to do whatever you feel is beneficial. I'm okay with everything.

koresar commented 4 years ago

FYI https://github.com/stampit-org/stamp/commit/0e89c6dfc3e55a8013bba3e49bccb17553854449#diff-b9cfc7f2cdf78a7f4b91a753d10865a2

PopGoesTheWza commented 4 years ago

At this point, I am still experiencing with an eslint setup and it is a playground for me to see how each plugin works on the codebase.

eslint-plugin-import supports ES, CommonJS syntax and is setup for the later
eslint-plugin-node is likely not critical. So far it threw an deprecated API and a few other I have not yet looked in detail

'events.EventEmitter.listenerCount' was deprecated since v4.0.0. Use 'events.EventEmitter#listenerCount()' instead.

eslint-plugin-jest is mostly to introduce me to the world of Jest and unit testing :)

Having (too many) lint rules activated to start with is often to me a way to pin point my bad habits and improper use of the language.

Finally, I believe we share the same view about tooling (bundlers, linters, taskrunners, etc.) They are only worthy if they add value to the production of the final product (source code and npm packages in this context.) With the scope being strictly limited to modern NodeJS and the progress of the build chain tools, things should be easier than when this monorepo started, imho)

PopGoesTheWza commented 4 years ago

@koresar I forked a few branches (one per package) with some refactored code. For each one, jest runs with no error.

Noteworthy is the eventemitable branch where a deprecated node API is fixed.

I tend to refactor aggressively, so see it not as a future PR but as an opportunity to define what I do right and what I do wrong :)

koresar commented 4 years ago

Lovely! 😀

On Thu., 7 Nov. 2019, 07:12 PopGoesTheWza, notifications@github.com wrote:

@koresar https://github.com/koresar I forked a few branches (one per package) with some refactored code. For each one, jest runs with no error.

Noteworthy is the eventemitable branch where a deprecated node API is fixed.

I tend to refactor aggressively, so see it not as a future PR but as an opportunity to define what I do right and what I do wrong :) [image: image] https://user-images.githubusercontent.com/32041843/68334049-1ac1cb00-00da-11ea-894b-9d483d89654b.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/issues/58?email_source=notifications&email_token=AAMMEL24JZ5S2P5WMDBW6VLQSMQK5A5CNFSM4JI2YBGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDH2ZIA#issuecomment-550481056, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMELZB2ULTNSD3HN5FR6LQSMQK5ANCNFSM4JI2YBGA .

PopGoesTheWza commented 4 years ago

@koresar if I open PRs from each branch to the eslint branch, would it make it easier for you?

koresar commented 4 years ago

That would make it harder. 😂 Just one PR for all is simpler I think. 👍

On Thu., 7 Nov. 2019, 09:37 PopGoesTheWza, notifications@github.com wrote:

@koresar https://github.com/koresar if I open PRs from each branch to the eslint branch, would it make it easier for you?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/stampit-org/stamp/issues/58?email_source=notifications&email_token=AAMMEL4QD5EPOT3YQHMBHSDQSNBLTA5CNFSM4JI2YBGKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEDIHO6Q#issuecomment-550532986, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMMEL2MIQZKFGXZFKSJE2DQSNBLTANCNFSM4JI2YBGA .

stampit-org / stamp

Switching from `jshint` to `eslint` #58