Repository navigation
fix: bump js-jod/core to 6 and update a few other packages - #29
Conversation
lavere
left a comment
There was a problem hiding this comment.
Verdict: comment; 🔴 0 🟠 1 🟢 2 ❓ 0
| # | Repo | Issue | Severity |
|---|---|---|---|
| 1 | fm-data-api-client | isolatedModules turns off the only type check in CI | 🟠 Should-fix |
| 2 | fm-data-api-client | Peer range skips js-joda 6.0.x | 🟢 Nit |
| 3 | fm-data-api-client | Typo in the commit title that goes into the changelog | 🟢 Nit |
Bumps @js-joda/core to 6.1.0 (dev) and widens the peer range to include 6.x, updates Biome, commitlint, fetch-mock, lint-staged and ts-jest, and turns on isolatedModules. The js-joda 6 changes (IE11 removal, locale packaging) do not touch the four classes this library uses, and build, lint, tsc and all 65 tests pass locally at this head. One side effect of isolatedModules is worth fixing before merge: nothing in CI type-checks the code any more.
package.json
🟢 Nit: Typo in the commit title that goes into the changelog
The commit and PR title say js-jod/core. semantic-release (release.config.js) writes this fix: line into CHANGELOG.md and the GitHub release notes, so it may be worth fixing to @js-joda/core before merge (or in the squash title).
Checked locally at this head: npm ci, npm run build, npm run lint, npx tsc --noEmit and npm test (65/65) all pass. npm ci reports 0 vulnerabilities. Every lockfile resolved URL points at registry.npmjs.org. The new handlebars and uglify-js entries come from ts-jest 29.4 and are dev-only.
js-joda 6: the 6.0.0 and 6.1.0 release notes list IE11 removal, a simpler nativeJs, locale packaging changes and bug fixes in Instant, Duration and zone parsing. src/Utils.ts only uses DateTimeFormatter.ofPattern, LocalDate, LocalTime and LocalDateTime parse/format, so none of it applies.
Good things: the isolatedModules flag silences the ts-jest TS151002 warning that the nodenext module setting from @tsconfig/node22 triggers on every test file.
Skipped dimensions: no Jira ticket in the title, so no AC check. No design mock (library). Learned reviewer patterns: Archon unreachable, skipped. The configured rules shelf targets app API and React code, so the standards check ran against this repo's own config and CI instead.
No prior comments or reviews on this PR (re-read just before posting).
| }, | ||
| "peerDependencies": { | ||
| "@js-joda/core": "^3.0.0 || ^4.0.0 || ^5.0.0" | ||
| "@js-joda/core": "^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.1.0" |
There was a problem hiding this comment.
🟢 Nit: Peer range skips js-joda 6.0.x
The new peer range is ^6.1.0, so a project pinned to @js-joda/core 6.0.0 or 6.0.1 gets an npm ERESOLVE peer conflict on install. The older entries use ^N.0.0, and nothing in 6.1.0 changes the parse/format calls src/Utils.ts uses. If 6.0.x was left out on purpose, ignore this. If not, ^6.0.0 matches the rest of the range.
|
I assume a switch to temporal would require bumping min node version to 26 plus a major release, correct? |
isolatedModules puts ts-jest in transpile-only mode, so npm test no longer catches type errors. Run tsc --noEmit in PR CI next to lint.
lavere
left a comment
There was a problem hiding this comment.
Verdict: approve; 🔴 0 🟠 0 🟢 2 ❓ 0
| # | Repo | Issue | Severity |
|---|---|---|---|
| 1 | fm-data-api-client | Peer range skips js-joda 6.0.x | 🟢 Nit |
| 2 | fm-data-api-client | Typo in the commit title that goes into the changelog | 🟢 Nit |
The new commit adds a typecheck script (tsc --noEmit) and runs it in PR CI next to Lint, which fixes the one should-fix from the last round. I checked it catches a real error: with const bad: number = "not a number"; added to src/Utils.ts, npm run typecheck fails with TS2322, and the tsconfig include covers both src and test. The two nits from last time are still open and do not block.
Fixed since the last review:
isolatedModules turns off the only type check in CI
Still open from the last review:
- 🟢 Nit: Peer range skips js-joda 6.0.x
- 🟢 Nit: Typo in the commit title that goes into the changelog
Checked at this head: npm ci and npm run typecheck pass. CI is green on Node 22 and 24, and the Typecheck step ran in both jobs.
Skipped dimensions: no Jira ticket in the title, so no AC check. No design mock (library). Learned reviewer patterns: Archon unreachable, skipped. The configured rules shelf targets app API and React code, so the standards check ran against this repo's own config and CI.
DASPRiD's question about Temporal and the minimum Node version is for the author, so this review does not answer it.
DASPRiD
left a comment
There was a problem hiding this comment.
This is a dependency refresh. It also adds a typecheck step to CI and turns on isolatedModules. The lockfile changes match the manifest bumps, and I found no source code that isolatedModules would break (src/index.ts only re-exports values). The one real question is the new @js-joda/core peer range: it skips the 6.0.x line, and nothing in the repo explains why.
| }, | ||
| "peerDependencies": { | ||
| "@js-joda/core": "^3.0.0 || ^4.0.0 || ^5.0.0" | ||
| "@js-joda/core": "^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.1.0" |
There was a problem hiding this comment.
Non-blocking: Peer range skips @js-joda/core 6.0.x
The new range ^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.1.0 accepts every 3.x, 4.x and 5.x release but only 6.1 and later. A consumer pinned to @js-joda/core@6.0.x gets an unmet-peer warning from npm, or an ERESOLVE failure when npm runs with strict peer deps, even though src/Utils.ts only uses DateTimeFormatter, LocalDate, LocalDateTime and LocalTime. If 6.0 has a known problem, say so in the PR description. Otherwise use ^6.0.0 to match the other majors. The README's "@js-joda/core 3.0 or higher" also reads as if 6.0 is supported.
## [3.0.2](v3.0.1...v3.0.2) (2026-10-01) ### Bug Fixes * bump js-jod/core to 6 and update a few other packages ([#29](#29)) ([696877e](696877e))
No description provided.