Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize our npm scripts #14827

Closed
nzakas opened this issue Jul 23, 2021 · 19 comments
Closed

Standardize our npm scripts #14827

nzakas opened this issue Jul 23, 2021 · 19 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion build This change relates to ESLint's build process chore This change is not user-facing
Projects

Comments

@nzakas
Copy link
Member

nzakas commented Jul 23, 2021

We have a number of different projects that are using different conventions for various npm scripts. I'd like us to standardize our script names and what they for the following actions:

  • Building - in packages where building is necessary, I'd like to standardize on build. I think we are doing this everywhere, but wanted to include here for the record.
  • Linting - I'd like to standardize on lint to lint every type of file, and then lint:*, where * is the file extension, for linting just specific types of files (examples: lint:js, lint:md).
  • Fixing lint errors - throughout our packages we have fix, fixlint, lint:fix, and some have no lint fix script at all. I'd like to standardize on lint:fix, which should run on all file types, or else lint:fix:* to run just on a subtype.
  • Testing - we are all over the place with this. In some packages test runs both linting and testing. In some packages we have unit:* scripts and in some we have test:* scripts. Some packages have coverage and others do not (test:cover, coverage, test-cov, etc.). I think we should standardize:
    1. That test does not include lint
    2. That we do test coverage
    3. That all testing that can have coverage does have coverage
    4. That test always runs with coverage
    5. That any subset of testing be named test:*. Every test:* is run with test

Thoughts?

@nzakas nzakas added build This change relates to ESLint's build process chore This change is not user-facing labels Jul 23, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Jul 23, 2021
@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Jul 23, 2021
@bmish
Copy link
Sponsor Member

bmish commented Jul 23, 2021

I agree with pretty much all of that.

I follow the Ember blueprint, which demonstrates a functional setup for running multiple lint/test scripts in parallel:

"build": "ember build --environment=production",
"lint": "npm-run-all --aggregate-output --continue-on-error --parallel \"lint:!(fix)\"",
"lint:fix": "npm-run-all --aggregate-output --continue-on-error --parallel lint:*:fix",
"lint:hbs": "ember-template-lint .",
"lint:hbs:fix": "ember-template-lint . --fix",
"lint:js": "eslint . --cache",
"lint:js:fix": "eslint . --fix",
"start": "ember serve",
"test": "npm-run-all lint test:*",
"test:ember": "ember test"

In summary:

  • Using the file extension (lint:js, lint:md)
  • If fixing is available, adding :fix after each script
  • lint and test should be independent scripts, and CI should call each of them separately

@snitin315
Copy link
Contributor

Makes sense to me 👍🏻

@mdjermanovic
Copy link
Member

My thoughts:

  • There should be a command that runs all validations we have in a project (linting, unit tests, license check, etc.). npm test seems to me like the expected way to check everything that should be checked, so I think that npm test should run linting.
  • lint:js doesn't have to mean literally only *.js, there is no reason to run ESLint separately for *.cjs files.
  • ESLint should be run with eslint ., that seems the best way to not miss linting newly added files/directories. Instead of specifying files/directories that should be linted in the command, we can specify files/directories that shouldn't be linted in .eslintignore or ignorePatterns config property.

@mdjermanovic
Copy link
Member

"build": "ember build --environment=production",
"lint": "npm-run-all --aggregate-output --continue-on-error --parallel \"lint:!(fix)\"",
"lint:fix": "npm-run-all --aggregate-output --continue-on-error --parallel lint:*:fix",
"lint:hbs": "ember-template-lint .",
"lint:hbs:fix": "ember-template-lint . --fix",
"lint:js": "eslint . --cache",
"lint:js:fix": "eslint . --fix",
"start": "ember serve",
"test": "npm-run-all lint test:*",
"test:ember": "ember test"

In summary:

  • Using the file extension (lint:js, lint:md)

  • If fixing is available, adding :fix after each script

  • lint and test should be independent scripts, and CI should call each of them separately

lint and test are two scripts, but not independent since test runs lint? By this commit message it seems they're of an opinion that npm test should fail if linting fails.

@bmish
Copy link
Sponsor Member

bmish commented Jul 23, 2021

@mdjermanovic Oh, I didn't notice that the Ember blueprint test script was running both lint and test. I guess that's the convention then so I'm fine either way. It's just slightly inconvenient when you want to run tests locally but not linting.

My thoughts:

  • There should be a command that runs all validations we have in a project (linting, unit tests, license check, etc.). npm test seems to me like the expected way to check everything that should be checked, so I think that npm test should run linting.
  • lint:js doesn't have to mean literally only *.js, there is no reason to run ESLint separately for *.cjs files.
  • ESLint should be run with eslint ., that seems the best way to not miss linting newly added files/directories. Instead of specifying files/directories that should be linted in the command, we can specify files/directories that shouldn't be linted in .eslintignore or ignorePatterns config property.

Agreed with all of that.

One more point, I like to ensure unused disable directive comments don't sneak in. Here are different ways to do that:

  1. "lint:js": "eslint --report-unused-disable-directives .",
  2. Adding reportUnusedDisableDirectives: true to .eslintrc.js
  3. Using eslint-plugin-eslint-comments's recommended config (including eslint-plugin-eslint-comments/no-unused-disable)

Any preference? Historically, I've used eslint-plugin-eslint-comments because it has a bunch of other good recommended rules around comment practices.

@mdjermanovic
Copy link
Member

One more point, I like to ensure unused disable directive comments don't sneak in. Here are different ways to do that:

  1. "lint:js": "eslint --report-unused-disable-directives .",
  2. Adding reportUnusedDisableDirectives: true to .eslintrc.js
  3. Using eslint-plugin-eslint-comments's recommended config (including eslint-plugin-eslint-comments/no-unused-disable)

Any preference? Historically, I've used eslint-plugin-eslint-comments because it has a bunch of other good recommended rules around comment practices.

We should use the built-in functionality for reporting unused disable directives: --report-unused-disable-directives and/or reportUnusedDisableDirectives: true.

--report-unused-disable-directives is preferable because it produces error severity (reportUnusedDisableDirectives: true produces warnings). However, it makes sense to use both at the same time, since reportUnusedDisableDirectives: true allows reporting unused directives in editors without any additional configuration. We have recently set reportUnusedDisableDirectives: true in eslint-config-eslint (#14699).

@github-actions
Copy link

Oops! It looks like we lost track of this issue. @eslint/eslint-tsc what do we want to do here? This issue will auto-close in 7 days without an update.

@github-actions github-actions bot added the Stale label Oct 15, 2021
@nzakas nzakas self-assigned this Oct 16, 2021
@pmcelhaney
Copy link
Contributor

I think this is a great idea and I'm willing to champion it. I'll do some research and add a comment here that fleshes out @nzakas's proposal and incorporates the suggestions in this thread. If you don't hear from me by August 1 please send me a gentle reminder. :)

@nzakas
Copy link
Member Author

nzakas commented Jul 21, 2022

@pmcelhaney much appreciated! 🙏 this has been in my list to circle back to.

@nzakas nzakas removed the Stale label Jul 21, 2022
@pmcelhaney
Copy link
Contributor

Here's a list of the current script names and where they're used, generated with this script.

build                              ;eslint.org, eslintrc, espree, playground
build:eleventy                     ;eslint.org, playground
build:sass                         ;eslint.org, playground
build:webpack                      ;eslint.org, playground
coveralls                          ;doctrine
docs:update-links                  ;eslint
eslint:github-action               ;github-action
fetch                              ;eslint.org
fetch:sponsors                     ;eslint.org
fetch:stats                        ;eslint.org
fetch:team                         ;eslint.org
fix                                ;eslint, eslint.org, eslintrc, playground
fix:docsjs                         ;eslint
fixlint                            ;espree
fuzz                               ;eslint
generate-alpharelease              ;create-config, doctrine, eslint, eslint-plugin-markdown, eslint-transforms, eslintrc, espree, github-action, typescript-eslint-parser
generate-betarelease               ;create-config, doctrine, eslint, eslint-plugin-markdown, eslint-transforms, eslintrc, espree, github-action, typescript-eslint-parser
generate-rcrelease                 ;create-config, doctrine, eslint, eslint-plugin-markdown, eslint-transforms, eslintrc, espree, github-action, typescript-eslint-parser
generate-release                   ;create-config, doctrine, eslint, eslint-plugin-markdown, eslint-transforms, eslintrc, espree, github-action, typescript-eslint-parser
generate-transcript                ;tsc-meetings
gensite                            ;eslint
images                             ;eslint.org, playground
install:playground                 ;eslint.org, playground
integration-tests                  ;typescript-eslint-parser
jest                               ;typescript-eslint-parser
kill-integration-test-containers   ;typescript-eslint-parser
lint                               ;create-config, doctrine, eslint, eslint-canary, eslint-cli, eslint-plugin-markdown, eslint-tester, eslint-transforms, eslint.org, eslintrc, espree, github-action, playground, tsc-meetings, typescript-eslint-parser
lint:docsjs                        ;eslint
lint:fix                           ;tsc-meetings
major                              ;eslint-tester
minor                              ;eslint-tester
new-rule-format                    ;eslint-transforms
patch                              ;eslint-tester
perf                               ;eslint
postinstall                        ;playground
postversion                        ;eslint-cli
prepare                            ;eslint-plugin-markdown, eslintrc
prepublishOnly                     ;espree
pretest                            ;doctrine, espree
publish-release                    ;create-config, doctrine, eslint, eslint-plugin-markdown, eslint-transforms, eslintrc, espree, github-action, typescript-eslint-parser
start                              ;eslint.org, playground
sync-docs                          ;espree
test                               ;create-config, doctrine, eslint, eslint-canary, eslint-plugin-markdown, eslint-tester, eslint-transforms, eslintrc, espree, github-action, typescript-eslint-parser
test-cov                           ;eslint-plugin-markdown
test:cli                           ;eslint
test:cov                           ;create-config
unit                               ;espree
unit:cjs                           ;espree
unit:esm                           ;espree
update-tag                         ;github-action
update-version                     ;espree
validate-projects                  ;eslint-canary
watch:eleventy                     ;eslint.org, playground
watch:sass                         ;eslint.org, playground
watch:webpack                      ;eslint.org, playground
webpack                            ;eslint

@pmcelhaney
Copy link
Contributor

pmcelhaney commented Jul 31, 2022

Here's a proposed standard which fleshes out @nzakas's original request.

ESlint Package Script Naming Conventions

Names

npm script names MUST contain only lower case letters, : to separates parts, - to separate words, and + to separate file extensions. Each part name SHOULD be either a full English word (e.g. coverage not cov) or a well-known initialism in all lowercase (e.g. wasm).

Here is a summary of the proposal in EBNF.

name = life-cycle | main ":fix"? target? option* ":watch"?

life-cycle = prepare | preinstall | install | postinstall | prepublish | preprepare | prepare | postprepare | prepack | postpack | prepublishOnly;

main = "build" | "lint" | "start" | "test";

target = ":" word ("-" word)* | extension ("+" extension)*;

option = ":" word ("-" word)*;

word = [a-z]+;

extension = [a-z0-9]+;

Order

The script names MUST appear in the package.json file in alphabetical order. The other conventions outlined in this document ensure that alphabetical order will coincide with logical groupings.

Main Script Names

With the exception of npm life cycle scripts all script names MUST begin with one of the following names.

Build

Scripts that generate a set of files from source code and / or data MUST have names that begin with build.

If a package contains any build:* scripts, there MAY be a script named build. If so, SHOULD produce the same output as running each of the build scripts individually. It MUST produce a subset of the output from running those scripts.

Lint

Scripts that statically analyze files (mostly, but not limited to running Eslint itself) MUST have names that begin with lint.

If a package contains any lint:* scripts, there SHOULD be a script named lint and it MUST run all of the checks that would have been run if each lint:* script was called individually.

If fixing is available, a linter MUST NOT apply fixes UNLESS the script contains the :fix modifier (see below).

Start

A start script is used to start a server. As of this writing, no ESLint package has more than one start script, so there's no need start to have any modifiers.

Test

Scripts that execute code in order to ensure the actual behavior matches expected behavior MUST have names that begin with test.

If a package contains any test:* scripts, there SHOULD be a script named test and it MUST run of all of the tests that would have been run if each test:* script was called individually.

A test script SHOULD NOT include linting.

A test script SHOULD report test coverage when possible.

Modifiers

One or more of the following modifiers MAY be appended to the standard script names above. If a target has modifiers, they MUST be in the order in which they appear below (e.g. lint:fix:js:watch not lint:watch:js:fix)

Fix

If it's possible for a linter to fix problems that it finds, add a copy of the script with :fix appended to the end that also fixes.

Target

The name of the target of the action being run. In the case of a build script, it SHOULD identify the build artifact(s), e.g. "javascript" or "css" or "website". In the case of a lint or test script, it SHOULD identify the item(s) being linted or tested. In the case of a start script, it SHOULD identify which server is starting.

A target MAY refer to a list of affected file extensions (such as cjs or less) delimited by a +. If there is more than one extension, the list SHOULD be alphabetized. When a file extension has variants (such as cjs for CommonJS and mjs for ESM), the common part of the extension MAY be used instead of explicitly listing out all of the variants (e.g. js instead of cjs+jsx+mjs).

The target SHOULD NOT refer to name of the name of the tool that's performing the action (eleventy, webpack, etc.)

Options

Additional options that don't fit under the other modifiers.

Watch

If a script watches the filesystem and responds to changes, add :watch to the script name.


What's next? Does this need to be an RFC? Or can I create a pull request under https://github.com/eslint/eslint/tree/main/docs/src/developer-guide ?

Once the standard is agreed upon, I'm happy to start sending PRs to update each of the package.json files and code that references the script names.

@pmcelhaney
Copy link
Contributor

@nzakas
Copy link
Member Author

nzakas commented Aug 10, 2022

I like the proposal 👍 Thanks for putting this together.

If others agree, I think we can just document it under Developer Guide and then move forward with PRs to other repos.

@eslint/eslint-tsc what do you think?

@btmills
Copy link
Member

btmills commented Aug 14, 2022

This is great! Since this isn't an external API, it's easy to change as we learn, so I agree there's no need for an RFC. Thanks @pmcelhaney!

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Aug 15, 2022
@nzakas nzakas moved this from Feedback Needed to Ready to Implement in Triage Aug 15, 2022
@nzakas
Copy link
Member Author

nzakas commented Aug 15, 2022

Marking as accepted. @pmcelhaney please proceed. 👍

mdjermanovic added a commit that referenced this issue Dec 14, 2022
* chore: standardize npm script names per #14827

* put lint:fix right after lint

* chore: fix ordering of :fix

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* chore: change release scripts to have "release:" prefix

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>

* docs: update package.json naming to include 'release'

* docs: add css so abnf code formats okay

* chore: exclude linting from npm test

* docs: remove counter-reset. I guess that was a merge error

Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Dec 24, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.29.0` -> `8.30.0`](https://renovatebot.com/diffs/npm/eslint/8.29.0/8.30.0) |

---

### Release Notes

<details>
<summary>eslint/eslint</summary>

### [`v8.30.0`](https://github.com/eslint/eslint/releases/tag/v8.30.0)

[Compare Source](eslint/eslint@v8.29.0...v8.30.0)

#### Features

-   [`075ef2c`](eslint/eslint@075ef2c) feat: add suggestion for no-return-await ([#&#8203;16637](eslint/eslint#16637)) (Daniel Bartholomae)
-   [`7190d98`](eslint/eslint@7190d98) feat: update globals ([#&#8203;16654](eslint/eslint#16654)) (Sébastien Règne)

#### Bug Fixes

-   [`1a327aa`](eslint/eslint@1a327aa) fix: Ensure flat config unignores work consistently like eslintrc ([#&#8203;16579](eslint/eslint#16579)) (Nicholas C. Zakas)
-   [`9b8bb72`](eslint/eslint@9b8bb72) fix: autofix recursive functions in no-var ([#&#8203;16611](eslint/eslint#16611)) (Milos Djermanovic)

#### Documentation

-   [`6a8cd94`](eslint/eslint@6a8cd94) docs: Clarify Discord info in issue template config ([#&#8203;16663](eslint/eslint#16663)) (Nicholas C. Zakas)
-   [`ad44344`](eslint/eslint@ad44344) docs: CLI documentation standardization ([#&#8203;16563](eslint/eslint#16563)) (Ben Perlmutter)
-   [`293573e`](eslint/eslint@293573e) docs: fix broken line numbers ([#&#8203;16606](eslint/eslint#16606)) (Sam Chen)
-   [`fa2c64b`](eslint/eslint@fa2c64b) docs: use relative links for internal links ([#&#8203;16631](eslint/eslint#16631)) (Percy Ma)
-   [`75276c9`](eslint/eslint@75276c9) docs: reorder options in no-unused-vars ([#&#8203;16625](eslint/eslint#16625)) (Milos Djermanovic)
-   [`7276fe5`](eslint/eslint@7276fe5) docs: Fix anchor in URL ([#&#8203;16628](eslint/eslint#16628)) (Karl Horky)
-   [`6bef135`](eslint/eslint@6bef135) docs: don't apply layouts to html formatter example ([#&#8203;16591](eslint/eslint#16591)) (Tanuj Kanti)
-   [`dfc7ec1`](eslint/eslint@dfc7ec1) docs: Formatters page updates ([#&#8203;16566](eslint/eslint#16566)) (Ben Perlmutter)
-   [`8ba124c`](eslint/eslint@8ba124c) docs: update the `prefer-const` example ([#&#8203;16607](eslint/eslint#16607)) (Pavel)
-   [`e6cb05a`](eslint/eslint@e6cb05a) docs: fix css leaking ([#&#8203;16603](eslint/eslint#16603)) (Sam Chen)

#### Chores

-   [`f2c4737`](eslint/eslint@f2c4737) chore: upgrade [@&#8203;eslint/eslintrc](https://github.com/eslint/eslintrc)[@&#8203;1](https://github.com/1).4.0 ([#&#8203;16675](eslint/eslint#16675)) (Milos Djermanovic)
-   [`ba74253`](eslint/eslint@ba74253) chore: standardize npm script names per [#&#8203;14827](eslint/eslint#14827) ([#&#8203;16315](eslint/eslint#16315)) (Patrick McElhaney)
-   [`0d9af4c`](eslint/eslint@0d9af4c) ci: fix npm v9 problem with `file:` ([#&#8203;16664](eslint/eslint#16664)) (Milos Djermanovic)
-   [`90c9219`](eslint/eslint@90c9219) refactor: migrate off deprecated function-style rules in all tests ([#&#8203;16618](eslint/eslint#16618)) (Bryan Mishkin)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC43MC40IiwidXBkYXRlZEluVmVyIjoiMzQuNzAuNCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1689
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@snitin315
Copy link
Contributor

I'll proceed with standardizing it in other repositories.

@snitin315 snitin315 self-assigned this Jun 10, 2023
@nzakas
Copy link
Member Author

nzakas commented Jun 13, 2023

Thank you @snitin315 🙏

@mdjermanovic
Copy link
Member

@snitin315 looks like we can close this now since all the projects have been updated?

@snitin315
Copy link
Contributor

Yes. This is done.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Feb 24, 2024
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion build This change relates to ESLint's build process chore This change is not user-facing
Projects
Archived in project
Triage
Ready to Implement
Development

No branches or pull requests

6 participants