netlify / eslint-config-node Goto Github PK

Instead of using the list of recommended rules from eslint-plugin-unicorn, we should manually enable each rule we want to use.
Like this, new versions of ESLint plugins do not add rules without us explicitly enabling them.

While I agree with this rule in principle, I find myself fighting with it a lot. It's particularly problematic in files where we pass lots of parameters around (see https://github.com/netlify/build/blob/master/.eslintrc.js#L38-L46).

In this particular case, I like how explicit we are about the parameters we receive and pass to each function. By doing this, we can be conservative about what we expose to other functions downstream. But this makes the code more verbose, which conflicts with this rule.

Any thoughts?

Which problem is this feature request solving?

See netlify/build@ed3775a#diff-21d7febb920de5511d3fd774505b2df8d3c295ff0b2ce579333c0b1c73cf0c80

Similar to the run-e script it would be useful to add run-local and possibly a run-ci script to only run commands locally/on CI

Describe the solution you'd like

Export a run-local scripts as a bin field so consumers can run a command only locally and not on CI.

Describe alternatives you've considered

Duplicating that code across repos

Can you submit a pull request?

Yes

Additional notes

Use https://github.com/watson/is-ci instead of process.env.CI

I find myself skipping this rule whenever I need to sort an array. Take the following example:

const applyConfigToFunction = ({ config, func }) => {
  // It's safe to mutate the array because it's local to this function.
  // eslint-disable-next-line fp/no-mutating-methods
  const matches = Object.keys(config)
    .filter((expression) => minimatch(func.name, expression))
    .map((expression) => {
      const starCount = [...expression].filter((char) => char === '*').length

      return {
        expression,
        weight: expression.length - starCount - (starCount ? 1 : 0),
      }
    })
    .sort((fA, fB) => fA.weight - fB.weight)
  const functionConfig = matches.reduce((result, { expression }) => mergeConfigObjects(result, config[expression]), {})

  return {
    [func.name]: functionConfig,
  }
}

In my view, this example adheres to the functional programming paradigm. The usage of sort does not cause any side effects. More importantly, there are no obvious alternatives to it.

What do you think?

I want to discuss this rule and why we are forbidding the use of for-of or for loops.

https://github.com/jfmengels/eslint-plugin-fp/blob/master/docs/rules/no-loops.md

In general if you need something performant a for loop is unbeatable on large scale

like

for (let i = 0, max = hugeSet.length(); i < max; i ++) {
...
}

a different example often you want to iterate over an iterate-able with entries in a sync manor where some async code is awaited:

for (const entry of entries) {
   await doSomeAsync(entry)
   console.log('done for entry')
}

IMO these are valid examples to use a for or forof and we should not forbid them.

cc @eduardoboucas @ehmicky @netlify-team-account-1 @ascorbic @erezrokah

When #368 was merged, I assume that removed the need to have specific config for React or Vanilla JS projects? Or is there some additional config that is still needed for projects of these types?

The following ruleset which is new causing issues:

• node/no-missing-require
• node/no-missing-import
• import/no-unresolved

In general they seem to be redundant (the ones from the node package with the ones from the import package)

This rule was causing our CI to fail on the publish of a release candidate because it requires installing dependencies. https://netlify.slack.com/archives/CBC2U9MMG/p1642520718015700

https://github.com/netlify/cli/runs/4855518297?check_suite_focus=true

This is not the major problem despite the fact that it increases our CI usage without the need to.

In my opinion which drives me crazy currently is if you work on multiple issues in parallel that have different additions on dependencies you cannot just switch to the other branch do a small typo fix and push.

(As on push the lint rule is run and complains about a non existing dependency)

So I have to run install (as I don't want to bypass it by running with --no-verify)

This is not a huge issue if you don't add or remove dependencies but if you do it can get quite frustrating.

This is an issue generated by (for-internal-use-only)github-tools

Description

This repository has exceeded the development grace period, and the repo owner must decide if branch protections should be enforced.
If this repository houses code used in production in any capacity, branch protections must be enforced. Read (for-internal-use-only)here for more details

What do I need to do?

• You can control whether or not branch protections are enforced by exclusively creating either of the labels:
  'enforce-branch-protections: true' or 'enforce-branch-protections: false'
• If there is no branch protection control label, this issue will be recreated.
• Once you have created either label, you can close this issue.
• If you have issues or questions, please reach out to #internal-security-n-compliance on slack.

I am proposing that this config is reduced to a more minimal and less opinionated set of rules. A lot of repos have been disabling many of these rules because they can be quite disruptive, and build has stopped using them entirely. Instead of more repos disabling more of these rules I propose we cut it down to a much more minimal set of rules:

1. remove fp, standard, you-dont-need-lodash-underscore and ava plugins. fp is too opinionated. standard doesn't have much point with prettier. We don't need eslint to tell us not to use lodash. Ava is only used on certain repos, which can add it as needed.
2. Change most of the other plugins to use their "recommmended" rule sets
3. enable a very limited set of unicorn rules. I suggets that these be limited to: ones that catch actual mistakes, and ones that suggest easier ways to do things that people might not know about.

For the Unicorn rules, if we go ahead I will do a list and we can discuss the choices.

There is an error with this repository's Renovate configuration that needs to be fixed. As a precaution, Renovate will stop PRs until it is resolved.

Error type: undefined. Note: this is a nested preset so please contact the preset author if you are unable to fix it yourself.

In compute we have some styling rules that are currently not enforced, but somewhat manually requested in PRs.
These two rules are:

1. Add an empty line before a return statement:

 something
+
 return x

2. Add an empty line after a variable declaration, unless next statement is also a variable declaration:

 var u = 3 
 const x = 1
 let y = 2
+
 something

In zip-it-and-ship-it we enabled the rule padding-line-between-statements with the following config

    'padding-line-between-statements': [
      'error',
      // Require newline before return
      { blankLine: 'always', prev: '*', next: 'return' },
      // Require newline after a batch of variable declarations
      { blankLine: 'always', prev: ['const', 'let', 'var'], next: '*' },
      { blankLine: 'any', prev: ['const', 'let', 'var'], next: ['const', 'let', 'var'] },
    ],

and this ensures that these exact two behaviors mentioned above are enforced.

Is this something that other teams are doing as well?
Should we enable this globally in this eslint config for all nodejs projects?
Are there any reasons against doing this?

I think if we can agree on this, this is one less thing to worry about when reviewing or writing nodejs code.

I brought this up in the dev tooling guild before and now here as a concrete issue.

cc @Skn0tt @eduardoboucas @ascorbic @khendrikse @lukasholzer @netlify/netlify-dev

We should consider adding a shared configuration for React/Browser applications.

Those might have a set of rules completely different than the ones this library provides, so we can also decide not to have them as a part of this library.

However, having everything at the same place can help with maintenance.

Should we start we something simple like:

  ...overrides,
  {
    files: ['*.jsx'],
    extends: ['eslint:recommended', 'plugin:react/recommended'],
  },

and see how it goes?

This issue lists Renovate updates and detected dependencies. Read the Dependency Dashboard docs to learn more.

Rate-Limited

These updates are currently rate-limited. Click on a checkbox below to force their creation now.

• fix(deps): update dependency eslint-plugin-react to v7.34.3
• fix(deps): update dependency eslint-plugin-promise to v6.2.0
• fix(deps): update dependency eslint-plugin-markdown to v5
• fix(deps): update dependency eslint-plugin-n to v17
• fix(deps): update dependency eslint-plugin-unicorn to v54
• fix(deps): update dependency execa to v9
• fix(deps): update dependency husky to v9
• fix(deps): update dependency npm-run-all2 to v6
• fix(deps): update typescript-eslint monorepo to v7 (major) (@typescript-eslint/eslint-plugin, @typescript-eslint/parser)
• chore(deps): lock file maintenance
• 🔐 Create all rate-limited PRs at once 🔐

Open

These updates have all been created already. Click a checkbox below to force a retry/rebase of any.

• fix(deps): update babel monorepo to v7.24.7 (@babel/core, @babel/eslint-parser)
• chore(deps): update dependency ava to v6
• chore(deps): update googlecloudplatform/release-please-action action to v4
• fix(deps): update commitlint monorepo to v19 (major) (@commitlint/cli, @commitlint/config-conventional)
• fix(deps): update dependency eslint to v9
• fix(deps): update dependency eslint-config-prettier to v9
• fix(deps): update dependency eslint-plugin-ava to v15
• fix(deps): update dependency eslint-plugin-cypress to v3
• fix(deps): update dependency eslint-plugin-html to v8
• fix(deps): update dependency prettier to v3
• Click on this checkbox to rebase all open PRs at once

Detected dependencies

• Check this box to trigger a request for Renovate to run again on this repository