Skip to content

fix: migrate to vitest, pin the release preset, and require @freckle/maybe 2.3.1 - #200

Open
joris974 wants to merge 8 commits into
mainfrom
chore/tooling-cleanup
Open

fix: migrate to vitest, pin the release preset, and require @freckle/maybe 2.3.1#200
joris974 wants to merge 8 commits into
mainfrom
chore/tooling-cleanup

Conversation

@joris974

@joris974 joris974 commented Aug 27, 2026

Copy link
Copy Markdown
Member

Eight commits, rebased onto main at #201. Three of the items in the original request were already on main after #199, so they are not here; see Already done below.

test: migrate from jest to vitest

Removes jest, jest-environment-jsdom, ts-jest and @types/jest; adds vitest. No config file: vitest's default include matches src/**/*.test.ts, and nothing in src/ touches a DOM API, so the default node environment works and jsdom is not needed. src/index.test.ts imports describe/expect/test from vitest rather than relying on globals, so no ambient test types are needed either.

Two things in tsconfig.json were resting on the jest stack, and both break if the dependencies are removed without touching it:

  • types was ["jest", "node"]. @types/node was never a direct devDependency — it arrived transitively through jest — so listing "node" stops resolving.

  • lib was never set, so it defaulted from target: ES2015. Object.entries is ES2017 and only type-checked because @types/node references a newer lib. Removing jest takes that away and src/filters.ts stops compiling:

    src/filters.ts(18,26): error TS2550: Property 'entries' does not exist on type
    'ObjectConstructor'. Do you need to change your target library?
    

    lib is now ES2020 explicitly. target stays ES2015, so the emit is unchanged; this only states the library level the source already assumed.

The migration would have silently dropped type-checking of tests

ts-jest type-checked test files as it ran them. vitest transforms with esbuild and does not type-check, and tsconfig.json excluded src/**/*.test.ts — so after a naive migration nothing would check them.

The exclude moves to a new tsconfig.build.json that build points at, tsconfig.json now covers src/ including tests, and a typecheck script runs tsc --noEmit over it. CI runs it between build and test. Verified it bites:

$ printf '\nconst broken: number = urlWithQueryParams("x", {})\n' >> src/index.test.ts
$ yarn typecheck
src/index.test.ts(81,7): error TS2322: Type 'string' is not assignable to type 'number'.
$ yarn build   # still passes — tsconfig.build.json excludes tests

dist/ is byte-identical after yarn build. Test time 4.23s → 136ms, the jsdom environment setup being most of the difference. yarn.lock goes 4348 → 1664 lines, devDependencies 7 → 4.

Dev-scoped advisories go from 17 to 12 (@ungap/structured-clone, inflight, whatwg-encoding and two brace-expansion highs clear). Being specific because it is easy to overstate: picomatch 4.0.3 (high, ReDoS) persists, moving from jest-util to vitest, and yarn npm audit --environment production reported no suggestions both before and after — the runtime tree was already clean, so this is not a consumer-facing security change.

ci: remove the obsolete PR checklist comment

The commenter-action posted a three-item checklist on every PR touching src/. All three are now impossible or enforced by CI:

  • "Flow interfaces in lib/ have been updated" — there is no lib/ directory and no Flow in this repo, and none in its history.
  • "Version in package.json is updated according to semver policy" — versions come from semantic-release. Hand-editing the version is the wrong thing to do, so the checklist was asking for a mistake.
  • "Built dist/ reflects all changes in PR prior to merge" — this is exactly what check-git-clean-action checks on every run.

Removes the workflow and .github/commenter.yml, which had no other consumer.

fix(release): pin conventional-changelog-conventionalcommits to v9

Listed bare, the preset is not a tracked dependency — npm resolves it fresh each run — and 10.x requires conventional-changelog-writer@9. The action supplies writer 8 via semantic-release@25@semantic-release/release-notes-generator@14, so generateNotes fails with Missing helper. Pinned to the major so 9.x fixes still flow. Same as freckle/maybe-js#162 and freckle/ajax-js#201.

Releases here are currently green, so this is preventive rather than a repair: because every run re-resolves the preset, the failure would arrive on whichever release ran after the tree shifted, unconnected to any change in this repo.

chore: drop deprecated jsxBracketSameLine from prettier config

Prettier warns jsxBracketSameLine is deprecated on every run. Its replacement is bracketSameLine, whose default of false matches the value set here, so formatting is unchanged and no replacement key is needed. No JSX in this package either way.

fix(build): tolerate a missing dist/

rm -r dist fails when dist/ is absent, so yarn build only worked because dist/ is committed:

$ rm -r dist
rm: dist: No such file or directory

rm -rf makes the clean step idempotent. Also drops the redundant -d, which duplicated "declaration": true in tsconfig.json. Verified by building from a tree with node_modules/ and dist/ both deleted.

fix(deps): require @freckle/maybe 2.3.1 — dropped, now on main

This branch carried a commit raising the @freckle/maybe floor from ^2.3.0 to ^2.3.1 (2.3.0 depended on lodash ^4.18.1; 2.3.1 has no dependencies). #201 made the same change, so the rebase dropped the commit as already upstream. The floor is ^2.3.1 on main and this PR no longer touches it.

The lodash entry in the lockfile is this package's own direct dependency, which is unchanged here:

$ yarn why lodash
└─ @freckle/query-params@workspace:.
   └─ lodash@npm:4.18.1 (via npm:^4.18.1)

docs: correct the breaking-change footer in RELEASE.md

The footer was documented as BREAKING CHANGES:, plural. The preset recognises only the singular forms, so anyone following RELEASE.md to cut a major got a patch release instead, with no error:

noteKeywords: ["BREAKING CHANGE","BREAKING-CHANGE"]
BREAKING CHANGE    -> notes: [{"title":"BREAKING CHANGE",...}]
BREAKING CHANGES   -> notes: NONE (not recognized)
BREAKING-CHANGE    -> notes: [{"title":"BREAKING-CHANGE",...}]

The <type>!: form documented alongside it does work, which is probably why this went unnoticed.

docs: document the API in the README and test: cover toQueryParamObj and createAPIQueryParams

The Usage section said TODO. Now documents the three exported functions with their null/array/Moment handling; every example was run against dist/ and shows real output.

It also documents the one thing a caller can get wrong without noticing — keys and values are not URL-encoded:

urlWithQueryParams('/api', {q: 'a&b=c'}) // '/api?q=a&b=c'  -> server reads two params
urlWithQueryParams('/api', {q: 'a#b'})   // '/api?q=a#b'    -> '#b' becomes a fragment

src/filters.ts had no tests at all, and createAPIQueryParams was only reached indirectly. The new cases cover the operator prefixing, array operators, the full operator set, the null-when-everything-dropped result, and pin the no-encoding behaviour so changing it means deliberately editing a failing test. 13 → 23 tests; verified 4 of them fail when the key format in toQueryParamObj is altered.

Already done, so not in this PR

Three requested items landed in #199 and are already on main:

  • check-git-clean-action is already in ci.yml.
  • The custom check script is already gone; there is no .sh file in the repo.
  • @semantic-release/git is already out of .releaserc.yaml.

Verification

Ran the sequence ci.yml defines from a tree with node_modules/ and dist/ deleted: yarn install --immutable, yarn build, yarn typecheck, yarn test (23 pass), then the action's git status --porcelain --untracked-files=normal, which comes back empty. dist/ is unchanged from main byte for byte. All four YAML files parse.

Re-run after the rebase onto #201, from a fresh clone: yarn install --immutable reports no lockfile drift, yarn build and yarn typecheck exit 0, 23 tests pass, the clean check is empty, and git diff origin/main -- dist is still empty.

Analysis: what is left, and what I deliberately did not do

Ordered by what I would pick up next. None of it is in this PR, because each needs its own release or its own decision.

1. No URL-encoding is a latent correctness bug. Values are interpolated raw, so a value containing &, =, #, + or a space changes the meaning of the URL instead of being escaped. Today's call sites in megarepo pass only ids, numbers, enum strings and timestamps, so nothing is broken right now — but QueryParamValueT accepts any string, so the first caller that passes free text (a search box, a name) corrupts the request silently. I checked: no caller pre-encodes, so adding encoding would not double-encode. It changes output for special characters, so it needs a major. This is the one item I would not leave indefinitely.

2. Drop the direct lodash dependency. src/filters.ts is the only user, and the whole of it reduces to one native line:

return Object.fromEntries(Object.entries(filters).map(([k, v]) => [`${name}[${k}]`, v]))

That removes lodash and @types/lodash and empties the tree of lodash entirely, now that @freckle/maybe 2.3.1 no longer pulls it. #197 now does exactly this; whichever of the two merges second will need a rebase, since both touch package.json and yarn.lock. Consumers relying on transitive lodash would break, which is why non-empty-js#240 shipped the equivalent change as a major — so this wants its own PR, not this one. (lib is already ES2020 here, so Object.fromEntries type-checks.)

3. moment-timezone for a type guard. The package uses exactly moment.isMoment(value) and value.valueOf(), and pays for the full tz database to get them. QueryParamValueT exports Moment in its union, so replacing it is breaking and worth bundling with items 1 and 2 into a single major.

4. The educator side has a forked copy. megarepo/frontend/educator/entities/ts/common/helpers/query-params.ts is a near-identical reimplementation with its own tests, while the student side imports this package. Two copies of URL-building logic will drift. Worth consolidating onto the published package.

5. No files field. The published tarball carries src/ and the tsconfigs. parser-js sets files: ["dist"]. Changing the published contract deserves its own PR.

6. setup-node pins no node-version. CI runs on whatever the runner defaults to, and release.yml has no setup-node step at all. Both work today; both will move without warning under us. There is no engines field or .nvmrc to pin against either.

7. renovate.json sets minimumReleaseAge: "0 days". Every release is eligible the moment it is published, with no soak time. Most compromised-package incidents are caught within a day or two, so a small delay is cheap insurance. This is org-wide config territory rather than a change to make here alone.

8. CI does not check formatting. .prettierrc and a format script exist, and restyled fixes style out of band, but nothing fails a PR for it. A prettier --check step would make the config self-enforcing. Low value while restyled is doing the job.

@joris974
joris974 requested a review from a team as a code owner August 27, 2026 21:45
@joris974
joris974 requested review from Origin-Slakman and z0isch and removed request for a team and Origin-Slakman August 27, 2026 21:45
@joris974 joris974 self-assigned this Aug 27, 2026
Removes jest, jest-environment-jsdom, ts-jest and @types/jest; adds vitest.

No config file: vitest's default include matches src/**/*.test.ts, and nothing
in src/ touches a DOM API, so the default node environment works and jsdom is
not needed. src/index.test.ts imports describe/expect/test from vitest rather
than relying on globals, so no ambient test types are needed either.

Two things in tsconfig.json were resting on the jest stack, and both break if
the dependencies are removed without touching it:

  - types was ["jest", "node"]. @types/node was never a direct devDependency;
    it arrived transitively through jest, so listing "node" stops resolving.

  - lib was never set, so it defaulted from target ES2015. Object.entries is
    ES2017 and only type-checked because @types/node references a newer lib.
    Removing jest takes that away and src/filters.ts stops compiling:

      src/filters.ts(18,26): error TS2550: Property 'entries' does not exist
      on type 'ObjectConstructor'.

    lib is now ES2020 explicitly. target stays ES2015, so the emit is
    unchanged; this only states the library level the source already assumed.

ts-jest type-checked test files as it ran them. vitest transforms with esbuild
and does not type-check, and tsconfig.json excluded src/**/*.test.ts, so the
migration would have left test files unchecked by anything. The exclude moves
to a new tsconfig.build.json that build now points at, tsconfig.json covers
src/ including tests, and a typecheck script runs tsc --noEmit over it. CI
runs it after build. Verified it fails on a deliberate error in the test file
while build still passes.

dist/ is byte-identical after yarn build. Test time 4.23s -> 136ms, the
jsdom environment setup being most of the difference.
The commenter-action posted a three-item checklist on every PR touching src/.
All three items are now either impossible or enforced by CI:

  - "Flow interfaces in lib/ have been updated": there is no lib/ directory
    and no Flow in this repo, and none in its history.

  - "Version in package.json is updated according to semver policy": versions
    come from semantic-release. Hand-editing the version is the wrong thing to
    do, so the checklist asked for a mistake.

  - "Built dist/ reflects all changes in PR prior to merge": this is what
    check-git-clean-action checks on every CI run, so a comment asking a human
    to remember it adds nothing.

Removes the workflow and .github/commenter.yml, which had no other consumer.
Listed bare, the preset is not a tracked dependency - npm resolves it fresh
each run - and 10.x requires conventional-changelog-writer@9. The action
supplies writer@8 via semantic-release@25 ->
@semantic-release/release-notes-generator@14, so generateNotes fails with
"Missing helper".

Pinned to the major so 9.x fixes still flow. Matches freckle/maybe-js#162 and
freckle/ajax-js#201.

Releases here are currently green, so this is preventive: every run resolves
the preset afresh, so the failure arrives on whichever release happens to run
after the tree shifts, not on a change to this repo.
Prettier warns "jsxBracketSameLine is deprecated" on every run. Its
replacement is bracketSameLine, whose default of false matches the value set
here, so formatting is unchanged and no replacement key is needed. There is no
JSX in this package either way.
rm -r dist fails when dist/ is absent, so yarn build only worked because
dist/ is committed. Anyone who removed it - or ran the build in a tree where
it had not been restored - got:

  rm: dist: No such file or directory
  error Command failed with exit code 1.

rm -rf makes the clean step idempotent. Also drops the redundant -d flag,
which duplicated "declaration": true in tsconfig.json.
The footer was documented as `BREAKING CHANGES:`, plural. The conventionalcommits
preset recognises only `BREAKING CHANGE` and `BREAKING-CHANGE`:

  $ node -e "... new CommitParser(await preset()).parse(...)"
  noteKeywords: ["BREAKING CHANGE","BREAKING-CHANGE"]
  BREAKING CHANGE    -> notes: [{"title":"BREAKING CHANGE",...}]
  BREAKING CHANGES   -> notes: NONE (not recognized)
  BREAKING-CHANGE    -> notes: [{"title":"BREAKING-CHANGE",...}]

So anyone following RELEASE.md to cut a major got a patch release instead, with
no error. The `<type>!:` form documented alongside it does work, which is
probably why this went unnoticed.
The Usage section said "TODO". Documents the three exported functions with
their null/array/Moment handling, and adds a section on the one behaviour a
caller can get wrong without noticing: keys and values are interpolated
without URL-encoding, so a value containing & or # silently changes the
meaning of the URL rather than being escaped.

Every example was run against dist/ and shows real output.
src/filters.ts had no tests at all, and createAPIQueryParams was only reached
indirectly through urlWithQueryParams. Adds cases for the operator prefixing,
array operators, the full operator set, and the null-when-everything-dropped
result that makes urlWithQueryParams return a bare baseUrl.

Also pins the no-encoding behaviour now documented in the README, so changing
it has to be a deliberate edit to a failing test rather than a silent change
in output.

13 -> 23 tests. Verified the new cases fail when the key format in
toQueryParamObj is altered.
@joris974
joris974 force-pushed the chore/tooling-cleanup branch from 1e1cd6f to 08a05b0 Compare August 28, 2026 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant