Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .npmrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
ignore-scripts=true
Comment thread
esezen marked this conversation as resolved.
Comment thread
esezen marked this conversation as resolved.
Comment thread
esezen marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important Issue: ignore-scripts=true blocks all lifecycle scripts for all packages, including those that require them to function correctly after a fresh install. The most relevant example in this repo is esbuild (a devDependency declared in package.json), which uses a postinstall script to download the correct platform binary. With ignore-scripts=true, a fresh npm install on a new machine will produce a non-functional esbuild binary, and npm run bundle will fail silently or with a confusing error.

The PR description acknowledges this with the lifecycle-script-allowed[] workaround, but that syntax (lifecycle-script-allowed) is not a real npm config key — it does not exist in any npm version. The valid approaches are:

  1. Use npm install --ignore-scripts=false esbuild after initial install (manual, error-prone for new contributors)
  2. Move esbuild to use a wrapper package that doesn't rely on install scripts (e.g., esbuild-wasm)
  3. Use a .npmrc per-package override via //registry.npmjs.org/:_authToken style or a tool like pnpm that has a proper allowlist
  4. Accept the tradeoff and document it clearly, then require contributors to re-run npm install --ignore-scripts=false esbuild when needed

At minimum, the README note should be expanded to mention that esbuild may need manual install if it's not already cached in node_modules.

save-exact=true
Comment thread
constructor-claude-bedrock[bot] marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: save-exact=true pins exact versions when running npm install <pkg> but has no effect on the already-committed package.json — existing entries like "esbuild": "^0.25.0" keep their caret ranges. This setting only changes behavior for future npm install <new-package> commands run by developers.

If the goal is truly reproducible builds, the existing package-lock.json already provides that guarantee — save-exact=true is complementary but not sufficient on its own to harden the existing entries. Consider whether the intent is:

  • Enforcing exact pins for future additions (this setting does that — acceptable, just document it)
  • Pinning all current entries (would require updating package.json ranges to exact versions — a separate, larger change)

This is a low-risk setting but the distinction should be made clear in the PR description or a follow-up comment.

3 changes: 3 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,11 @@ window.addEventListener('cio.client.search.getSearchResults.completed', (event)
## Development / npm commands

```bash
npm run prepare # one-time after cloning: install husky git hooks (pre-push lint)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The comment says "one-time after cloning" but npm run prepare should also be re-run after npm ci in a clean CI-like environment (since ignore-scripts=true will suppress it). Consider rewording to make it clear this is required any time node_modules is re-created from scratch:

npm run prepare       # run once after npm install (sets up husky git hooks)

Also, since this is a client-side browser library consumed by downstream projects, it is worth noting that the .npmrc will affect consumers who clone the repo, but ignore-scripts=true does not propagate to consumers who install the published npm package — so there is no downstream impact for library users.

npm run lint # run lint on source code and tests
npm run test # run tests
npm run coverage # run tests and serves coverage reports from localhost:8081
npm run docs # output documentation to `./docs` directory
```

> **Note:** `.npmrc` sets `ignore-scripts=true` for supply-chain safety, so lifecycle scripts (including husky's hook installation) do not run automatically on `npm install`. Run `npm run prepare` once after cloning to enable the `pre-push` lint hook.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important Issue: This note only mentions husky, but omits the more impactful case: esbuild's binary setup also relies on a postinstall script. A new contributor following these instructions exactly (npm install then npm run prepare) will have a broken esbuild and will hit a confusing failure when running npm run bundle or any build step.

The note should either:

  1. Explicitly call out that esbuild requires npm install --ignore-scripts=false esbuild after the initial install, OR
  2. If esbuild happens to be bundled without its native binary in the node_modules already committed (which would be unusual), clarify that.

Recommended expanded note:

> **Note:** `.npmrc` sets `ignore-scripts=true` for supply-chain safety. After `npm install`:
> 1. Run `npm run prepare` once to install the husky `pre-push` lint hook.
> 2. Run `npm install --ignore-scripts=false esbuild` if `npm run bundle` fails (esbuild requires its postinstall script to download the platform binary).

Loading