chore: simplify and standardize `package.json` scripts and CI setup #7069

serhalp · 2025-02-27T14:14:10Z

📣 You should probably review this commit by commit!

`2593f33` Remove `postinstall` hook that builds the package

It's nonstandard
It's surprising
Since we use tsc to build, it causes every CI check to fail if you have type errors
It causes issues when rewriting git history
It's a bit magical

I refactored an affected script in a separate commit first (be729a2), for easier review.

`b57d815` Simplify site build scripts

There were two separate scripts for doing the same thing for some reason. I ran into this oddity while making the above change.

`5c3f767` Remove leftover refs to `ava`

ava was removed a couple years ago

`f4119f8` Rename misleading CI jobs

They handle two kinds of benchmarks, only one of which is package size

`bf2bfa2` Split formating, linting, and unit testing

They were all jumbled together 😢
CI wouldn't run unit tests if you had a prettier or eslint error
The scripts were nonstandard and surprising
The scripts were unnecessarily complicated
CI was running formatting and linting on Windows, macOS, and Linux for no reason

`846d1c4` Simplify CI workflow skipping for release branches

You can just skip the whole job instead of skipping every step!

`7169000` chore(package.json): sort scripts

This one is dedicated to @ndhoule. Keep fighting the good fight ✌🏼.

`ec2e860` Add missing dependency on `npm-run-all`

We're using run-s which comes from this dep but it only happens to work because one of
our dependencies has a transitive dependency on npm-run-all.

IMO we should kill this, but for now this just adds the missing dep.

Note: npm-run-all2 is a maintained fork of the unmaintained npm-run-all

github-actions · 2025-02-27T14:15:17Z

📊 Benchmark results

Comparing with fb36998

Dependency count: 1,206 (no change)
Package size: 293 MB (no change)
Number of ts-expect-error directives: 729 (no change)

serhalp · 2025-02-27T14:16:03Z

package.json

    "site:build": "run-s site:build:*",
    "site:build:install": "cd site && npm install --no-audit",
    "site:build:assets": "cd site && npm run build",
    "postinstall-pack": "node ./scripts/postinstall.js",


This postinstall-pack hook gets renamed to postinstall in a script triggered from the prepublishOnly hook, so that when users install this package they see some output we want them to see at install time.

serhalp · 2025-02-27T14:16:56Z

package.json

    "site:build:install": "cd site && npm install --no-audit",
    "site:build:assets": "cd site && npm run build",
    "postinstall-pack": "node ./scripts/postinstall.js",
-    "postinstall": "npm run build && node ./scripts/postinstall.js",


See comment above – we were also triggering this when running npm i in this repo when developing in this repo, but that seems unnecessary so I didn't bother preserving it.

... In fact this may be why we had the preinstall build step in the first place... so that this script can run (it uses some npm deps).

There were two separate scripts for doing the same thing.

serhalp · 2025-02-27T18:55:48Z

CONTRIBUTING.md

 ```

 ### Attaching a Debugger



Whoops, my editor reformatted this. I can revert it if the reviewer cares.

serhalp · 2025-02-27T18:56:43Z

CONTRIBUTING.md

-npm install && npm run site:build:install
+npm install


This being mentioned here didn't make any sense so I removed it (there's a section below about the docs site)

serhalp · 2025-02-27T18:57:25Z

package.json

    "test:ci:vitest:integration": "vitest run --coverage tests/integration/",
    "e2e": "node ./tools/e2e/run.js",
-    "docs": "npm run --prefix=site build",
+    "site:build": "npm run build && npm install --prefix=site --no-audit && npm run --prefix=site build",


I don't love the npm install inside a script but that's a whole can of worms that didn't seem worth pursuing

No behavioural changes in this commit.

- It's nonstandard - It's surprising - Since we use `tsc` to build, it causes every CI check to fail if you have type errors - It causes issues when rewriting git history - It's a bit magical

ava was removed a couple years ago

They handle two kinds of benchmarks, only one of which is package size

We're using `run-s` which comes from this dep but it only happens to work because one of our dependencies has a transitive dependency on `npm-run-all`. IMO we should kill this, but for now this just adds the missing dep. Note: npm-run-all2 is a maintained fork of the unmaintained npm-run-all

serhalp · 2025-02-27T19:53:30Z

package.json

-    "test": "run-s format test:dev",
-    "format": "run-s format:check-fix:*",
-    "format:ci": "run-s format:check:*",
-    "format:check-fix:lint": "run-s format:check:lint format:fix:lint",


bonus win: this was running eslint twice... fixing also checks, so it was redundant!

serhalp · 2025-02-27T19:53:38Z

package.json

-    "format:check-fix:lint": "run-s format:check:lint format:fix:lint",
-    "format:check:lint": "cross-env-shell eslint $npm_package_config_eslint",
-    "format:fix:lint": "cross-env-shell eslint --fix $npm_package_config_eslint",
-    "format:check-fix:prettier": "run-s format:check:prettier format:fix:prettier",


same for prettier

ndhoule

… setup (#7069)" This reverts commit 94cb80a.

…I setup (#7069)" This reverts commit 94cb80a.

The CLI currently runs `npm run build` in a post-install script. Many of commands in the CLI implicitly depend on this behavior, which is unnecessarily complicated. This post-install script conflicts with the user-facing post-install script, which means we need to do a little pre- publish post-install script juggling to ensure that the end user runs the correct script after installing the CLI. This change removes the post-install script in favor of explicitly building the CLI before e.g. CI steps that require a built CLI. Partial un-reversion of #7069, which was reverted in #7085.

serhalp commented Feb 27, 2025

View reviewed changes

serhalp force-pushed the chore/remove-postinstall-hook branch 2 times, most recently from 06768a9 to b914412 Compare February 27, 2025 18:29

chore(package.json): simplify site build

b57d815

There were two separate scripts for doing the same thing.

serhalp force-pushed the chore/remove-postinstall-hook branch from b914412 to da1f74b Compare February 27, 2025 18:51

serhalp commented Feb 27, 2025

View reviewed changes

CONTRIBUTING.md

```

### Attaching a Debugger

Copy link

Member Author

serhalp Feb 27, 2025

Choose a reason for hiding this comment

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

Whoops, my editor reformatted this. I can revert it if the reviewer cares.

serhalp commented Feb 27, 2025

View reviewed changes

serhalp added 7 commits February 27, 2025 13:59

refactor(scripts): split up prepare-for-publish

be729a2

No behavioural changes in this commit.

chore: remove bizarre postinstall hook that builds

2593f33

- It's nonstandard - It's surprising - Since we use `tsc` to build, it causes every CI check to fail if you have type errors - It causes issues when rewriting git history - It's a bit magical

chore: remove leftover refs to ava

5c3f767

ava was removed a couple years ago

ci: rename misleading jobs

f4119f8

They handle two kinds of benchmarks, only one of which is package size

chore: split formating, linting, and unit testing

bf2bfa2

ci: simplify workflow skipping for release branches

846d1c4

chore(package.json): sort scripts

7169000

serhalp force-pushed the chore/remove-postinstall-hook branch from da1f74b to 946bd2d Compare February 27, 2025 19:38

serhalp changed the title ~~chore: remove bizarre postinstall hook that builds~~ chore: simplify and standardize package.json scripts and CI setup Feb 27, 2025

serhalp force-pushed the chore/remove-postinstall-hook branch from 946bd2d to ec2e860 Compare February 27, 2025 19:48

serhalp marked this pull request as ready for review February 27, 2025 19:48

serhalp requested a review from a team as a code owner February 27, 2025 19:48

serhalp commented Feb 27, 2025

View reviewed changes

ndhoule approved these changes Feb 27, 2025

View reviewed changes

Merge branch 'main' into chore/remove-postinstall-hook

1b778f0

serhalp enabled auto-merge (squash) February 27, 2025 22:54

serhalp merged commit 94cb80a into main Feb 28, 2025
49 checks passed

serhalp deleted the chore/remove-postinstall-hook branch February 28, 2025 11:42

serhalp mentioned this pull request Feb 28, 2025

ci: fix new job config #7071

Merged

ndhoule added a commit that referenced this pull request Mar 4, 2025

Revert "chore: simplify and standardize package.json scripts and CI…

1579ae4

… setup (#7069)" This reverts commit 94cb80a.

ndhoule added a commit that referenced this pull request Mar 4, 2025

revert: "chore: simplify and standardize package.json scripts and C…

67a4a41

…I setup (#7069)" This reverts commit 94cb80a.

ndhoule added a commit that referenced this pull request Mar 4, 2025

revert: "chore: simplify and standardize package.json scripts and C…

4571be3

…I setup (#7069)" This reverts commit 94cb80a.

token-generator-app bot mentioned this pull request Mar 4, 2025

chore(main): release 19.0.1 #7062

Merged

ndhoule mentioned this pull request Mar 20, 2025

build: remove build step from post-install hook #7119

Merged

ndhoule mentioned this pull request Mar 25, 2025

style: alphabetize package.json scripts #7128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

chore: simplify and standardize `package.json` scripts and CI setup #7069

chore: simplify and standardize `package.json` scripts and CI setup #7069

serhalp commented Feb 27, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Feb 27, 2025 •

edited

Loading

Uh oh!

serhalp Feb 27, 2025

Uh oh!

serhalp Feb 27, 2025

Uh oh!

serhalp Feb 27, 2025

Uh oh!

serhalp Feb 27, 2025

Uh oh!

serhalp Feb 27, 2025

Uh oh!

serhalp Feb 27, 2025

Uh oh!

serhalp Feb 27, 2025

Uh oh!

serhalp Feb 27, 2025

Uh oh!

ndhoule left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

chore: simplify and standardize package.json scripts and CI setup #7069

chore: simplify and standardize package.json scripts and CI setup #7069

Conversation

serhalp commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

2593f33 Remove postinstall hook that builds the package

b57d815 Simplify site build scripts

5c3f767 Remove leftover refs to ava

f4119f8 Rename misleading CI jobs

bf2bfa2 Split formating, linting, and unit testing

846d1c4 Simplify CI workflow skipping for release branches

7169000 chore(package.json): sort scripts

ec2e860 Add missing dependency on npm-run-all

Uh oh!

github-actions bot commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Benchmark results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndhoule left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

chore: simplify and standardize `package.json` scripts and CI setup #7069

chore: simplify and standardize `package.json` scripts and CI setup #7069

serhalp commented Feb 27, 2025 •

edited

Loading

`2593f33` Remove `postinstall` hook that builds the package

`b57d815` Simplify site build scripts

`5c3f767` Remove leftover refs to `ava`

`f4119f8` Rename misleading CI jobs

`bf2bfa2` Split formating, linting, and unit testing

`846d1c4` Simplify CI workflow skipping for release branches

`7169000` chore(package.json): sort scripts

`ec2e860` Add missing dependency on `npm-run-all`

github-actions bot commented Feb 27, 2025 •

edited

Loading

ndhoule left a comment •

edited

Loading