v2.0.0: Full TypeScript modernization#10
Merged
Merged
Conversation
Replace Gulp/Browserify/Mocha with tsup/Vitest/Biome. package.json v2.0.0, ESM+CJS dual exports, Node 20+. tsconfig.json strict, ES2022 target. Assisted-by: 🤖 claude-opus-4-6@default
Eid, exceptions, preconditions rewritten as typed classes. ES2022 Error.cause support in all exception constructors. tryToExecute preserves original error as cause. Assisted-by: 🤖 claude-opus-4-6@default
59 tests, 100% coverage. Mocha+expect.js replaced with Vitest. Added cause chaining tests for exceptions and tryToExecute. Assisted-by: 🤖 claude-opus-4-6@default
Node 20+22 matrix, lint/test/build jobs. Concurrency control, timeout protection. Assisted-by: 🤖 claude-opus-4-6@default
Drop gulp/, bower.json, .travis.yml, dist/, gulpfile.js. Drop old lib/ and test/ (replaced by src/). Assisted-by: 🤖 claude-opus-4-6@default
Defensive programming framing, ESM/CJS/browser examples. Error.cause docs, rewritten EidPreconditions section. Drop Bower, update badges to GitHub Actions. Assisted-by: 🤖 claude-opus-4-6@default
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe project is rewritten from legacy JavaScript/Gulp modules to a TypeScript library with EID formatting, typed exceptions, precondition helpers, ESM/CJS/IIFE builds, Vitest coverage, Biome linting, updated packaging, CI, and documentation. ChangesEID modernization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Application
participant EidPreconditions
participant Eid
participant EidRuntimeException
Application->>EidPreconditions: checkArgument or tryToExecute
EidPreconditions->>Eid: normalize eid
EidPreconditions->>EidRuntimeException: create typed exception
EidRuntimeException-->>Application: throw with formatted eid and cause
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
tsconfig.json (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude tests for type checking.
Since
tsupstrictly bundles thesrc/index.tsentry point and does not rely ontsconfig.json's exclusion rules to shape thedistdirectory, you can safely dropsrc/**/*.test.tsfrom theexcludearray. This allowstsc --noEmitto also validate the types inside your test suites.🛠️ Proposed fix
"include": ["src"], - "exclude": ["node_modules", "dist", "src/**/*.test.ts"] + "exclude": ["node_modules", "dist"] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tsconfig.json` around lines 20 - 22, Update the tsconfig.json exclude list by removing the src/**/*.test.ts pattern, while preserving node_modules and dist exclusions, so tsc --noEmit type-checks the test suites.src/__tests__/preconditions.test.ts (1)
189-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the exact boundary condition for
checkElementIndex.To prevent regressions of the off-by-one bounds logic, consider adding a test case that specifically verifies that
index === sizeappropriately throws anEidIndexOutOfBoundsException.🧪 Proposed test case addition
describe("giving a invalid index value of 6, and size of 2", () => { it("should throw EidIndexOutOfBoundsException", () => { let caught: unknown; try { EidPreconditions.checkElementIndex(6, 2, eid); } catch (e) { caught = e; } expect(caught).toBeInstanceOf(EidIndexOutOfBoundsException); expect(String(caught)).toMatch(eidRegex); }); }); + + describe("giving a boundary index value of 2, and size of 2", () => { + it("should throw EidIndexOutOfBoundsException", () => { + let caught: unknown; + try { + EidPreconditions.checkElementIndex(2, 2, eid); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(EidIndexOutOfBoundsException); + expect(String(caught)).toMatch(eidRegex); + }); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/__tests__/preconditions.test.ts` around lines 189 - 201, Add a boundary-focused test alongside the existing invalid-index case in the `checkElementIndex` tests, invoking `EidPreconditions.checkElementIndex` with `index` equal to `size` and asserting it throws `EidIndexOutOfBoundsException` with the expected `eidRegex` message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Line 19: Update all checkout steps in .github/workflows/ci.yml at lines 19-19,
35-35, and 49-49 by adding a with block that sets persist-credentials to false;
apply the same configuration to each actions/checkout@v4 step.
- Around line 1-13: Add a top-level permissions block to the CI workflow
configuration, granting only contents: read. Place it alongside the existing
workflow-level name, triggers, and concurrency settings so all jobs inherit the
least-privilege permission without changing job behavior.
In `@package.json`:
- Around line 25-35: Add a dedicated type-check script to the package scripts
using tsc --noEmit, then include that script in the prepublishOnly pipeline
alongside lint, test, and build so TypeScript errors are validated before
publishing.
- Around line 10-18: Update the package exports map so the import condition
resolves the ESM declaration file and the require condition resolves the
tsup-generated CJS declaration file. Keep the existing runtime targets
unchanged, and place each condition-specific types entry within its
corresponding import or require branch rather than using one shared top-level
types path.
In `@src/eid.ts`:
- Around line 28-32: Update format in the EID class to replace template
placeholders in a single pass rather than reducing with sequential
String.replace calls. Ensure injected argument values containing “%s” are not
rescanned or consumed as placeholders, while each original template placeholder
uses its corresponding argument.
In `@src/index.ts`:
- Around line 38-48: Update the backward-compatibility block in src/index.ts to
explicitly assign the existing preconditions and exceptions objects onto the Eid
class/default export as Eid.preconditions and Eid.exceptions, while preserving
the named exports and current exception mappings.
In `@src/preconditions.ts`:
- Around line 45-47: Update isIndexAndSizeIllegal so the upper-bound check
rejects indices equal to size by using an inclusive boundary, while preserving
rejection of negative indices.
---
Nitpick comments:
In `@src/__tests__/preconditions.test.ts`:
- Around line 189-201: Add a boundary-focused test alongside the existing
invalid-index case in the `checkElementIndex` tests, invoking
`EidPreconditions.checkElementIndex` with `index` equal to `size` and asserting
it throws `EidIndexOutOfBoundsException` with the expected `eidRegex` message.
In `@tsconfig.json`:
- Around line 20-22: Update the tsconfig.json exclude list by removing the
src/**/*.test.ts pattern, while preserving node_modules and dist exclusions, so
tsc --noEmit type-checks the test suites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3954088a-59b5-4050-849e-31abd003ca9d
⛔ Files ignored due to path filters (5)
dist/browser/index.htmlis excluded by!**/dist/**dist/browser/toplevel/eid.jsis excluded by!**/dist/**dist/browser/toplevel/eid.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/browser/toplevel/eid.min.js.mapis excluded by!**/dist/**,!**/*.map,!**/*.min.js.mappackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
.github/workflows/ci.yml.gitignore.travis.ymlREADME.mdbiome.jsonbower.jsongulp/build.jsgulp/build/toplevel.jsgulp/clean.jsgulp/config.jsgulp/serve.jsgulp/tests.jsgulpfile.jslib/eid.jslib/eid/eid.jslib/eid/exceptions.jslib/eid/preconditions.jspackage.jsonsrc/__tests__/eid.test.tssrc/__tests__/exceptions.test.tssrc/__tests__/preconditions.test.tssrc/eid.tssrc/exceptions.tssrc/index.tssrc/preconditions.tstest/eid.jstest/eid/exceptions.jstest/eid/preconditions.jstsconfig.jsontsup.config.tsvitest.config.ts
💤 Files with no reviewable changes (16)
- lib/eid.js
- bower.json
- .travis.yml
- test/eid/exceptions.js
- gulp/build/toplevel.js
- gulp/clean.js
- gulp/build.js
- lib/eid/exceptions.js
- gulp/tests.js
- gulpfile.js
- gulp/config.js
- gulp/serve.js
- test/eid.js
- test/eid/preconditions.js
- lib/eid/eid.js
- lib/eid/preconditions.js
CI: add permissions: contents: read, persist-credentials: false. package.json: nested types in exports for CJS/ESM, add typecheck script. eid.ts: fix %s injection in JFormatter (single-pass replace). preconditions.ts: fix off-by-one in checkElementIndex (index >= size). index.ts: remove misleading backward compat block. Assisted-by: 🤖 claude-opus-4-6@default
Attach preconditions and exceptions as static properties on Eid class. Legacy consumers using Eid.preconditions.checkNotNull() still work. Assisted-by: 🤖 claude-opus-4-6@default
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.
Complete rewrite of eid.js for 2026.
Changes
tryToExecutepreserves original error as.causeexportsfield, IIFE bundle for browsers via unpkgBreaking changes
importinstead ofrequire, CJS still supported)tryToExecutenow sets caught error as.cause(previously stringified into message only)Assisted-by: 🤖 claude-opus-4-6@default
Summary by CodeRabbit
New Features
Documentation
Chores