Skip to content

update eslint configuration#397

Closed
legobeat wants to merge 15 commits into
LavaMoat:mainfrom
legobeat:project-eslint
Closed

update eslint configuration#397
legobeat wants to merge 15 commits into
LavaMoat:mainfrom
legobeat:project-eslint

Conversation

@legobeat

@legobeat legobeat commented Nov 10, 2022

Copy link
Copy Markdown
Collaborator
  • start linting sources with config based on @metamask/eslint-config-nodejs.
  • consolidate eslint path selection and ignores in .eslintIgnore (fun fact: !-negative-filters don't work elsewhere)
  • fix linting (broken out in Prelint cleanup #397 #399 )

outstanding questions:

  • what goes in .depcheckrc.ignores and what goes in devDependencies?
    • Can we normalize this across subpackages or are some intended to be more independent?
    • Is there a reason to use separate approach for ava vs eslint?

@legobeat legobeat marked this pull request as ready for review November 10, 2022 14:14
@legobeat legobeat marked this pull request as draft November 10, 2022 14:46
@legobeat legobeat mentioned this pull request Nov 10, 2022
"lint:deps": "depcheck"
},
"engines": {
"node": ">=12.0.0"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@kumavis

This was added to address a node/no-unsupported-features/node-builtins linting error. However, I think there's more to this.

It looks like some of the code actually uses some features not in node v12, alread (note: would be good to address those don't get caught in linting, too).

One straight-forward way to resolve this would be to add a constraint to engines.node: ">=14.x" is added to package.json for each subpackage. So how to treat that comes down to if support for node v12 is actually still desired at this point.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Addressed with separate proposal in #398

const JSONStream = require('JSONStream')
const through = require('through2')
const umd = require('umd')
// eslint-disable-next-line node/prefer-global/buffer

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Is there some subtlety making the require('buffer') necessary? If not it seems better to just remove it.

@legobeat legobeat mentioned this pull request Nov 11, 2022
@legobeat legobeat force-pushed the project-eslint branch 3 times, most recently from 1665f33 to dab4caf Compare November 15, 2022 04:19
@legobeat

Copy link
Copy Markdown
Collaborator Author

Added rules:

  • quotes=[error,single] (because it saves you pressing shift as much)
  • semis=[error,never] (already normative in repo)

@legobeat legobeat marked this pull request as ready for review November 18, 2022 10:51
@legobeat legobeat requested review from a team and kumavis November 18, 2022 10:52
@kumavis kumavis mentioned this pull request Nov 21, 2022
@kumavis

kumavis commented Nov 21, 2022

Copy link
Copy Markdown
Member

merged in #409

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.

2 participants