Add ESLint to the project

[soryy708]

This adds an ESLint configuration per #24
The rules (incl. style) are derived from the existing practices in the project.
Simple inconsistencies have been fixed in a separate commit in this pull request.
Running the linter (npm run lint) results in empty output.

[nickbabcock]

Thank you for opening the PR and kicking it off! I'll move our discussion here as this is where the code is.

Extending the recommended is reasonable, but it doesn't cover many very useful checks. Also, the "recommended" may be working on a style that's different from the existing convention here.

I'm ok with changing the style of this project to conform to best practices.

The linter configuration can be quite long, especially if you add a lot of plugins.

A big linter configuration is something I'd like to avoid. I'd prefer to keep it as simple as possible.

From what I've seen so far, looks like the common practice is to put it in its own file (.eslintrc) as opposed to in package.json.

Projects like preact put it in package.json. Unless there is a consensus, I'd prefer package.json

Why do you care about cluttering the root directory? You can #26 if you're concerned about what happens when the package is npm installed.

It's more of a personal preference. In general, I find the root directory is becoming a bit dumping grounds for dotfiles:

• .babelrc
• .dockerignore
• .editorconfig
• .eslintignore
• .eslintrc
• .npmrc
• .npmignore
• .prettierrc

All these files detract from the core of the project (the actual code). Especially for small projects (like this one). Seeing a ton of configuration for small projects is (to me) a project smell. Linting, formatting, and configuration are all important but in small, low velocity projects a ton of customization can add more to the cognitive overhead than anything. Hence why I want to keep the number of config lines and config files to a minimum.

I'm reading the docs for eslint-plugin-node and it looks like a lot of these rules are already covered by the basic eslint. Which rules in particular do you want?

I'm reading the docs for eslint-plugin-prettier and it looks like a lot of rules are also already covered by the basic eslint, and some may conflict. Which rules in particular do you want?

My interest lies entirely in minimizing the number of lines and files dedicated for a good linting configuration. I'm open to suggestions. One doesn't need to specify the individual rules when using these extends.

Technically you can run a linter without any configuration per-project, so it runs with your personal global preferences. However, if you do it that way:

1. You're not sharing it with other developers
2. Other developers may have different preferences
3. The linting can't happen as part of CI/CD

I'm totally on board with defining a linting configuration and communicating style preferences and adding them to the project.

However, I do have reservations with committing a bunch of dependencies to the project. I noticed that this commit adds 1,409 lines to package-lock.json, which would literally double the number of lines. It was recently discussed why npm lockfiles can be a security blindspot for injecting malicious modules. So I'm hesitant on adding dependencies not pertinent to running, building, or testing the project.

I'm only interested in running the latest eslint (and relevant plugins dictated by the config) when one invokes npm run lint.

• Which ecmaVersion are we targeting? Is this supposed to be run in a browser? or in a node environment? How far back to we provide backwards compatibility?

jomini can be used in both browser and node.js. As long as jomini works on node LTS, I'm fine.

• no-prototype-builtins is part of eslint:recommended, but turning it on flags existing code as an error (specifically various calls to .hasOwnProperty()). Do we want to keep the rule off? or do we turn the rule on and fix the existing code?

If existing code is flagged, it should be fixed

• The comparison operator (==) may do implicit type coercion, but the strict comparison operator (===) doesn't do that. There are various instances of using == in the existing code when === could have been used. There's an option to turn on a rule to enforce that (eqeqeq).

• Do we care about magic numbers in the existing code? (no-magic-numbers)

• var declarations imply hoisting. There's a rule that enforces putting them on the top of the scope, so that the implied hoisting is communicated. (vars-on-top)

• var can be replaced with const or let in newer versions of JS. Do we want to enforce that? (no-var)

• Arrow callbacks are more concise and intuitive in terms of scope (e.g. this keyword semantics). Do we want to replace function(){}s with ()=>{}s where possible? (prefer-arrow-callback)

Since these are not flagged by ["eslint:recommended", "plugin:node/recommended", "prettier"] (the smallest configuration for best practices in our environment -- again, I'm open to suggestions here), I wouldn't worry about them.

---

Oftentimes when I'm unsure of the best approach, I see what others are doing. Taking a look at the prolific sindresorhus, his projects more often than not don't generate a package-lock.json file and use the xo linter (built ontop of eslint). Maybe this is something that we should explore. What do you think?

[soryy708]

A big linter configuration is something I'd like to avoid. I'd prefer to keep it as simple as possible.

Why?

Projects like preact put it in package.json. Unless there is a consensus, I'd prefer package.json

That would make package.json a long file. Is it better than adding a separate file which has its own concern?

My interest lies entirely in minimizing the number of lines and files dedicated for a good linting configuration. I'm open to suggestions.

If that's the case, we can make a separate project which defines the linter rules, and extend that.

One doesn't need to specify the individual rules when using these extends.

That's true.

• .eslintrc has some "off" rules which I left in as a reminder about the discussion here on whether they should be turned on or removed.
• I should check what rules I've written explicitly but are implicitly covered by the defaults, and remove them.

However, I do have reservations with committing a bunch of dependencies to the project.

These are dev dependencies. They don't impact production code using the library.

It was recently discussed why npm lockfiles can be a security blindspot for injecting malicious modules.

eslint has 7.9 million weekly downloads, and eslint-plugin-import has 4.6 million. Are you worried about security with this ammount of eyes watching it?

So I'm hesitant on adding dependencies not pertinent to running, building, or testing the project.

Technically linting is a form of testing.

I'm only interested in running the latest eslint (and relevant plugins dictated by the config) when one invokes npm run lint.

Personally I like ESLint to run in the background, and highlight linting errors to me as I type (through a VSCode extension). This is like intellisense.

jomini can be used in both browser and node.js. As long as jomini works on node LTS, I'm fine.

In that case we need to decide if we write in an old version of javascript and enforce it through linting, or we add a build step which performs transpilation (babel).
jomini uses commonjs modules (require and module.exports), which may not be supported by some browsers. This can be fixed with transpilation / browserification (browserify).

Since these are not flagged by ["eslint:recommended", "plugin:node/recommended", "prettier"] (the smallest configuration for best practices in our environment -- again, I'm open to suggestions here), I wouldn't worry about them.

eslint:recommended (and other recommendations) are the smallest because they don't cover all best practices. What they cover is IMHO arbitrary.
Some of the rules I listed are stylistic. I think we should insist on eqeqeq since implicit type coercion can make nasty bugs (JS flaws).
no-var would be nice if we decide to not support browsers that don't supoport let/const, or if we set up transpilation.

Taking a look at the prolific sindresorhus, his projects more often than not don't generate a package-lock.json file and use the xo linter

I am not familiar with xo. If you'd like to explore it further, go for it. What I don't like about it is:

• it's relative (to eslint) obscurity (only 14k dls per week as opposed to millions)
• staleness (last commit to master 4 months ago, as opposed to yesterday)

As a separate issue, transpilation & browserification can be handled by gulp scripts, or very easily by webpack.

[nickbabcock]

A big linter configuration is something I'd like to avoid. I'd prefer to keep it as simple as possible.

Why?

If given a choice of maintaining 400 lines or 1 line, I'd choose 1 line.

Projects like preact put it in package.json. Unless there is a consensus, I'd prefer package.json

That would make package.json a long file. Is it better than adding a separate file which has its own concern?

How would adding three lines making it a long file? 😕

This was my proposal:

{
  "eslintConfig": {
  	"extends": ["eslint:recommended", "plugin:node/recommended", "prettier"]
  }
}

My interest lies entirely in minimizing the number of lines and files dedicated for a good linting configuration. I'm open to suggestions.

If that's the case, we can make a separate project which defines the linter rules, and extend that.

You are more than welcome to do that.

It was recently discussed why npm lockfiles can be a security blindspot for injecting malicious modules.

eslint has 7.9 million weekly downloads, and eslint-plugin-import has 4.6 million. Are you worried about security with this ammount of eyes watching it?

No it's not eslint's lockfile I'm worried about reviewing, it's jomini's lockfile.

In that case we need to decide if we write in an old version of javascript and enforce it through linting, or we add a build step which performs transpilation (babel).
jomini uses commonjs modules (require and module.exports), which may not be supported by some browsers. This can be fixed with transpilation / browserification (browserify).

jomini can be used without issue in browsers with a bundler.

import { parse } from 'jomini';

console.log(JSON.stringify(parse('abc=123')));

Since these are not flagged by ["eslint:recommended", "plugin:node/recommended", "prettier"] (the smallest configuration for best practices in our environment -- again, I'm open to suggestions here), I wouldn't worry about them.

eslint:recommended (and other recommendations) are the smallest because they don't cover all best practices. What they cover is IMHO arbitrary.
Some of the rules I listed are stylistic. I think we should insist on eqeqeq since implicit type coercion can make nasty bugs (JS flaws).
no-var would be nice if we decide to not support browsers that don't supoport let/const, or if we set up transpilation.

I would like to only rely upon existing an eslint plugins that encapsulate best practices and refrain from personal preferences.

I am not familiar with xo. If you'd like to explore it further, go for it. What I don't like about it is:

• it's relative (to eslint) obscurity (only 14k dls per week as opposed to millions)
• staleness (last commit to master 4 months ago, as opposed to yesterday)

I'm not familiar with it either, but XO uses the latest version of eslint under the hood, and another synonym for staleness is stability.

But maybe xo isn't a good fit and lynt should be used. I'm unsure what the answer is, just trying to help out.

[soryy708]

If given a choice of maintaining 400 lines or 1 line, I'd choose 1 line.

It's configuration. Not really something you fix bugs in.

How would adding three lines making it a long file?

If we keep only minimal linting (eslint:recommended etc), it's not long.

This was my proposal:

{
  "eslintConfig": {
  	"extends": ["eslint:recommended", "plugin:node/recommended", "prettier"]
  }
}

I recall. I disagree with it.

No it's not eslint's lockfile I'm worried about reviewing, it's jomini's lockfile.

Jomini's lockfile is a superset of eslint lockfile.

jomini can be used without issue in browsers with a bundler.

Right now it can. Changes may accidentally break that. We can enforce it via eslint.

But maybe xo isn't a good fit and lynt should be used

Lynt is even worse than xo for the same reasons. 453 weekly downloads and last commit to master 11 months ago.

[soryy708]

If we look at eslint documentation about eslint:recommended:

• Most of the included rules are "possible errors"
• Most of the "best practices" are not included
• It doesn't enforce compatibility with browsers

Look at all the goodies eslint-plugin-import gives: https://www.npmjs.com/package/eslint-plugin-import

[soryy708]

Existing conventions that won't be enforced by eslint:recommended:

• quotes - enforce use of single quotes
• semi - require semicolons
• no-console
• curly - enforce consistent brace style for all control statements
• indent - indent with 2 spaces

and additional rules as seen in .eslintrc.

[soryy708]

Looks like jomini code is already ES3. I've changed the .eslintrc here to enforce ES3.

This has the effect of changing sourceType to script (the default) since module is supported only for ES2015+.

Looks like generated lib/jomini.js is also ES3 compliant, even though it does a few weird things.

[soryy708]

Which ecmaVersion are we targeting?
jomini can be used in both browser and node.js. As long as jomini works on node LTS, I'm fine.

I went ahead and made the linter enforce ES3. Existing code is already compliant.

Existing code is flagged as error by no-prototype-builtins. TODO: Fix existing code and enable the rule.

Taking a look at the prolific sindresorhus, his projects more often than not don't generate a package-lock.json file and use the xo linter

If you look at the rules enabled by extending xo, you'll see it targets bleeding edge ES versions. That's not fit for our purpose as you want backwards compat, and so far looks like we're going with ES3.
You'll also see it has plenty of rules that I proposed (see .eslintrc), and a few extra.

I understand we don't want to maintain transpilation / browserification as part of this project.

[soryy708]

I think all that's left to do at this point is to go over the rules I propose in addition to the recommended ones (eslint docs, eslint-plugin-import docs), and for each make a ruling on whether it stays in the config or gets disabled.

Reminder, existing stylistic conventions that I propose and won't be enforced by eslint:recommended:

• quotes - enforce use of single quotes
• semi - enforce semicolons
• curly - enforce consistent brace style for all control statements
• indent - indent with 2 spaces

[nickbabcock]

Looks like we're still in disagreement (which is fine!) on the best way to add eslint + configuration. So we'll table discussion until new ideas can be presented.

I'm still happy to accept your code changes here if you'd like to split those off into a separate PR.

[soryy708]

Do I understand correctly that you intend on accepting the changes as long as eslintConfig is kept minimal (only eslint:recommended and empty rules)?
I'll split my proposed configuration beyond minimal to a separate PR.

[nickbabcock]

Do I understand correctly that you intend on accepting the changes as long as eslintConfig is kept minimal (only eslint:recommended and empty rules)?
I'll split my proposed configuration beyond minimal to a separate PR.

Yeah minimal is good. I'm kinda flabbergasted that there isn't something like:

"extends": [
  "node-best-practices"
]

and that'd be it. (This is a bit of a rhetorical statement). Rust, go, c++, python need zero configuration to set up linting, yet JS needs configuration (if not 100's of lines of configuration)? 😕

And now I'm having second thoughts about the package-lock.json as I still can't get over how big of a change linting is. I'm thinking that maybe it's best to to set package-lock=false in .npmrc like you mentioned in the other issue. Though, funny enough the requests library stores it in the .gitignore!

This topic is rather exhausting for a code base that measures in at a 100 lines of code 😆

When I said

I'm still happy to accept your code changes here if you'd like to split those off into a separate PR.

I meant only the changes under lib/ and test/ (excluding .eslintrc)

[soryy708]

I'm kinda flabbergasted that there isn't something like:

"extends": [
  "node-best-practices"
]

"Best practices" are subjective, and the industry isn't exactly standardized.

I'm thinking that maybe it's best to to set package-lock=false in .npmrc like you mentioned in the other issue. Though, funny enough the requests library stores it in the .gitignore!

Actually, you remind me that package-lock.json is intended for apps but not libraries. Since jomini is a library, it probably doesn't need a package-lock. Doing it through .npmrc is smarter than through .gitignore. That's however irrelevant to this PR.

I meant only the changes under lib/ and test/ (excluding .eslintrc)

You mean the changes made to fix linting errors? I can make it in to a separate PR if you insist but I don't see the value in it.

[nickbabcock]

I'm happy to accept PRs that updates code to conform to some eslint rule, but as of now I have yet to see a solution that minimizes the amount of cruft that is added to the project. Any solution would need to be less than a half dozen lines.