Repository navigation
Develop - #4
Merged
Merged
Develop#4
Conversation
Bump TypeScript 5.9 -> 6.0.3, the highest major supported by @typescript-eslint v8 (peer range >=4.8.4 <6.1.0). TypeScript 7 is deliberately skipped: it falls outside that range, so linting would run against an unsupported compiler. TS 6 turns deprecated `baseUrl` into error TS5101, which broke the tsup DTS build - tsup hardcodes `baseUrl: compilerOptions.baseUrl || "."` in its dts pipeline and there is no released fix. Set `ignoreDeprecations` to "6.0" to restore the build; it is a no-op here since no `paths` are configured. Add overrides to clear 4 high-severity advisories in transitive deps of @huggingface/transformers, which pins all of them below the fixed versions with no upstream release available: sharp ^0.34.5 -> ^0.35.3 libvips CVEs (GHSA-f88m-g3jw-g9cj) adm-zip 0.5.x -> ^0.6.0 GHSA-xcpc-8h2w-3j85 Pin esbuild to 0.27.2 for GHSA-g7r4-m6w7-qqqr. tsup requires ^0.27.0 and every version in 0.27.3 - 0.28.0 is affected, leaving 0.27.2 as the only safe in-range choice. This must be an override rather than a lockfile pin because package-lock.json is gitignored, so a fresh install would otherwise re-resolve a vulnerable version. Also applies the pending in-range dev dependency updates (eslint, prettier, vitest, typescript-eslint, @types/node). Verified: npm audit reports 0 vulnerabilities after deleting the lockfile and reinstalling; typecheck, lint, build (incl. DTS), 801 tests and the live ONNX smoke test all pass.
The format script wrapped its globs in single quotes. POSIX shells strip these, but cmd and PowerShell do not, so prettier received the literal string 'src/**/*.ts', matched zero files, and exited 0 - the script was a silent no-op for anyone developing on Windows. Switching to escaped double quotes works on every platform, since both POSIX shells and cmd strip them. Add a format:check counterpart so the same globs can be enforced without writing, suitable for CI. Without it this failure mode can recur unnoticed.
No functional changes - formatting only. Because npm run format silently matched nothing on Windows (fixed in the previous commit), 90 files had been formatted with Prettier's defaults (printWidth 80, trailingComma "all") rather than the repo's .prettierrc (printWidth 100, trailingComma "es5"). This applies the configured style, mostly unwrapping call signatures and argument lists that fit within 100 columns. Verified: lint, typecheck, build and all 801 tests pass unchanged.
The lockfile was gitignored, so every contributor and CI run resolved dependencies independently. Builds were not reproducible, and a compromised or regressed transitive patch release could land silently without any diff to review. This is especially load-bearing now that three `overrides` are the only thing holding transitive dependencies at non-vulnerable versions; without a lockfile there is no record of what actually got installed. Committing it does not affect consumers of the published package - npm ignores lockfiles from dependencies - so the usual argument for omitting it in a library does not apply here. It also enables `npm ci` in CI.
eslint.config.mjs already declared `tests/**/*.ts` in its `files` glob,
but `npm run lint` only ever passed `src/`, so no test file had been
linted. Enabling it surfaced 34 errors.
Relax two rules for tests/ only, since the defaults are wrong for test
code rather than the code being wrong:
- no-explicit-any: off. Mocking fetch, setTimeout and provider
responses legitimately needs `any` (21 of the 34).
- no-unused-vars: allow the `^_` prefix convention for intentionally
unused args, which the suite was already using.
The remaining 12 were genuine dead code:
- Unused imports (SimilarityMetric, vi, beforeEach, afterEach).
- Unused mock params, now `_`-prefixed to match the convention.
- `originalSetTimeout`, captured to restore a global that vi.spyOn
already restores.
- A whole count=2 v1 buffer in serialize-v2.test.ts that the author's
own comments explain was abandoned - count=2 is ambiguous with
V2_VERSION - and replaced with the count=1 buffer below it. The
explanation is kept; only the dead buffer is removed.
- An EmbeddingStore the test creates and then deliberately bypasses in
favour of SearchIndex, as its comment describes.
Two were captured for a before/after comparison that was never written:
- `beforeRebalance` in incremental.test.ts: completed as intended by
asserting rebalance preserves total membership, an invariant the
sibling integration test already checks. It passes.
- `cohesionBefore` in incremental-pipeline.test.ts: removed rather than
completed. Asserting cohesion does not decrease across a rebalance is
not a guarantee the implementation makes, and the test already
asserts every post-rebalance cluster has non-zero cohesion.
Verified: lint, format:check and all 801 tests pass.
The repository had no CI, so nothing enforced the checks that already
existed. Formatting had silently drifted across 90 files and tests were
never linted, both of which went unnoticed for exactly this reason.
Three jobs:
verify format:check, lint, typecheck, test and build across
Node 20, 22 and 24.
engines-floor builds on Node 24, then runs the built dist on Node 18.
audit npm audit against the committed lockfile.
The verify matrix floor is Node 20 rather than the `engines.node` floor of
18, because the dev toolchain cannot run on 18 - vitest 4 requires
^20 || ^22 || >=24 and eslint 10 requires ^20.19 || ^22.13 || >=24. Rather
than drop the `>=18` claim or ship a knowingly red matrix entry, the
engines-floor job verifies the claim the way it actually matters: the
published artifact has no runtime dependencies, so scripts/engines-check.mjs
imports both the ESM and CJS builds on Node 18 and asserts real behaviour
from cosineSimilarity, dotProduct and topK. Keep it in step with
engines.node.
Installs use --ignore-scripts to skip the onnxruntime-node and sharp
postinstalls, which fetch hundreds of MB of native binaries that nothing
in CI needs; verified locally that a clean `npm ci --ignore-scripts`
passes every step. `npm run smoke` is deliberately excluded since it
downloads models from HuggingFace, and stays a local check.
audit is gated at --audit-level=high so moderate findings in dev-only
transitive dependencies cannot block a merge, and also runs weekly on a
schedule so new advisories surface on their own rather than failing an
unrelated pull request.
Documents the decisions made across the preceding commits, and corrects statements that they invalidated - notably the note claiming package-lock.json is gitignored, which is no longer true. - Give each of the three overrides an explicit "remove when" condition, so none of them silently outlives its reason. They force versions outside ranges their dependents declare, which is only defensible for as long as the advisory has no upstream fix. - Warn that `npm test` does not cover the sharp and adm-zip overrides: the suite mocks providers, so it stays green even if the local ONNX provider breaks. `npm run smoke` is the only guard. - Explain why esbuild has to be an override rather than a lockfile entry. - Describe the CI jobs, why the verify matrix floor is Node 20 while engines.node is >=18, and why installs pass --ignore-scripts. - Note the tests-only eslint relaxations, and that scripts/*.mjs is covered by neither eslint nor prettier. - Add an Unreleased CHANGELOG section stating plainly that none of this changes the public API or runtime behavior.
scripts/*.mjs was covered by neither eslint nor prettier, the last gap in the tooling. Add scripts/ to both, with a scripts-only eslint block: these files are plain ESM parsed by the default parser, so the typescript-eslint rules do not apply and only core no-unused-vars is enabled. Formatting them surfaced a second problem. scripts/engines-check.mjs showed as 84 changed lines for a 42-line file - every line - because it had been committed with CRLF while the rest of the tree is LF. Prettier normalized it, and `git ls-files --eol` found two more: .gitignore and .github/workflows/ci.yml. All three were added or edited in the preceding commits on Windows; the other 168 tracked files were already LF. Add .gitattributes with `* text=auto eol=lf` and renormalize. Prettier already enforces LF, but only for the extensions it covers, so it could never have caught .gitignore or ci.yml. A whole-file CRLF diff hides the real change during review, which is how this went unnoticed. Verified: format:check, lint, typecheck, 801 tests, engines:check and the live ONNX smoke test all pass; no CRLF remains in the index.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.