Skip to content

Drop r-result dependency in favour of an inlined Result type#26

Merged
silverbucket merged 2 commits into
masterfrom
refactor/inline-result-drop-r-result
Jul 4, 2026
Merged

Drop r-result dependency in favour of an inlined Result type#26
silverbucket merged 2 commits into
masterfrom
refactor/inline-result-drop-r-result

Conversation

@silverbucket

@silverbucket silverbucket commented Jul 4, 2026

Copy link
Copy Markdown
Owner

Summary

Removes the sole runtime dependency, r-result, by inlining the Result type. No breaking changes — the full r-result instance API is ported verbatim, so Result objects are behaviorally identical.

The connect() promise resolves to a Result (Ok/Fail). That was the only thing the package provided, so it's now defined directly in irc-socket.js.

No breaking changes

All 18 r-result instance methods are preserved with identical semantics:

ok, fail, isOk, isFail, map, mapFail, and, or, andThen, orElse, toArray, unwrapOr, unwrapOrElse, match, inspect, debug, debugOk, debugFail

  • Unwrapping the wrong variant throws a TypeError, with the same custom-message behavior.
  • The inspect method is ported verbatim, matching r-result exactly (including that modern Node no longer auto-invokes a string-named inspect for util.inspect — this is unchanged from r-result).

Verified with an exhaustive method-by-method behavioral check in addition to the existing suite.

Changes

  • irc-socket.js: inline the Ok/Fail Result type; drop require("r-result").
  • package.json / package-lock.json: remove the r-result dependency (the package now has zero runtime dependencies).
  • readme.md: update the description of the resolved value.

Tests

Full suite passes (51 passing). The existing tests exercise .ok(), .fail(), .isOk(), and .isFail() across success and every failure path; a separate exhaustive check covers the remaining methods (map, andThen, match, unwrapOr, etc.).

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@silverbucket, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b3f39f78-9a53-4cff-aa1a-764e97a9b47b

📥 Commits

Reviewing files that changed from the base of the PR and between 43f1323 and 54bdbf0.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • README.md
  • irc-socket.js
  • package.json
  • result.js
  • test/result.js
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/inline-result-drop-r-result

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The connect() promise resolves to a Result (Ok/Fail). This was the only
use of the r-result package, so inline the type and remove the runtime
dependency.

The full r-result instance API is ported verbatim -- ok, fail, isOk,
isFail, map, mapFail, and, or, andThen, orElse, toArray, unwrapOr,
unwrapOrElse, match, inspect, debug, debugOk, debugFail -- so Result
objects are behaviourally identical to r-result. No breaking change for
consumers.

Also updates the readme reference and removes r-result from
package.json / package-lock.json (the package now has zero runtime
dependencies).
@silverbucket
silverbucket force-pushed the refactor/inline-result-drop-r-result branch from 5b2cc7b to 2705323 Compare July 4, 2026 20:23
@silverbucket silverbucket self-assigned this Jul 4, 2026
@silverbucket

Copy link
Copy Markdown
Owner Author

@coderabbitai code review

@coderabbitai

coderabbitai Bot commented Jul 4, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
gh pr diff 26 --repo silverbucket/irc-socket-sasl

Length of output: 8325


🏁 Script executed:

#!/bin/bash
cd /tmp 2>/dev/null
rg -n "r-result|rresult" -g '!package-lock.json' . 2>/dev/null | head -50

Length of output: 1209


🏁 Script executed:

#!/bin/bash
pwd
ls
rg -n "r-result|rresult" -g '!package-lock.json'

Length of output: 476


🏁 Script executed:

#!/bin/bash
cat -n irc-socket.js | sed -n '1,40p'
echo "---"
grep -n "Ok(\|Fail(" irc-socket.js | head -60
echo "---tests---"
rg -n "r-result|require\(.irc-socket" test | head -20

Length of output: 3097


🏁 Script executed:

#!/bin/bash
fd . test
echo "---"
rg -n "map|andThen|match|unwrapOr|debug" test | head -40

Length of output: 1473


@silverbucket Reviewed the PR — this is a clean, well-executed dependency removal.

What I checked:

  • Confirmed there are no remaining references to r-result/rresult anywhere in irc-socket.js, package.json, package-lock.json, or README.md — the removal is complete.
  • Verified all 18 resultMethods (ok, fail, isOk, isFail, map, mapFail, and, or, andThen, orElse, toArray, unwrapOr, unwrapOrElse, match, inspect, debug, debugOk, debugFail) are present in the inlined type at Lines 19–204 of irc-socket.js, with logic matching the described r-result semantics (unwrap-wrong-variant throws TypeError, custom-message support preserved, inspect ported verbatim including the depth < 0 short-circuit and recursive opts handling).
  • Checked all call sites of Ok(...)/Fail(...) in the connect flow (e.g. Lines 398, 414, 437, 458, 489, 546, 557, 562, 573, 623, 633, 665) — all resolve the promise with the new inlined type as before, no behavior change.
  • Confirmed package.json/package-lock.json correctly drop the dependencies block containing r-result, and the lockfile no longer lists the node_modules/r-result entry. The package now truly has zero runtime dependencies.
  • Load-order check: Ok/Fail reference resultMethods which is declared after them in the file, but since neither factory function is invoked during module evaluation (only later, at runtime), there's no temporal-dead-zone issue — this is safe.
  • README.md update is accurate and adds a small usability note about res.isOk()/res.isFail()/res.ok()/res.fail().

One minor, non-blocking observation: the existing committed test suite (test/irc-socket.js) only exercises .ok(), .fail(), .isOk(), .isFail() through the normal connect flows — the "exhaustive method-by-method behavioral check" mentioned in the PR description for map/andThen/match/unwrapOr/etc. doesn't appear to be part of the committed test file. If that exhaustive check was done ad hoc during development, it might be worth adding as a permanent test file so future refactors of this inlined Result type stay protected by CI — but this is just a suggestion, not a blocker.

Overall: no functional issues found, LGTM.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Addresses the review suggestion on #26: the inlined Result type was only
exercised indirectly through connect() flows (ok/fail/isOk/isFail), so
the remaining methods (map/andThen/match/unwrapOr/inspect/debug/etc.)
had no permanent CI coverage.

Move the Result type from irc-socket.js into result.js and add
test/result.js, which covers every method plus a parity check asserting
the instance exposes exactly the r-result method set. No behaviour
change; irc-socket.js now requires ./result.js.
@silverbucket

Copy link
Copy Markdown
Owner Author

Addressed the review suggestion in 54bdbf0.

The inlined Result type is now extracted into its own module (result.js) and backed by a dedicated, committed test suite (test/result.js) that exercises every method — map, mapFail, and, or, andThen, orElse, toArray, unwrapOr, unwrapOrElse, match, inspect, debug/debugOk/debugFail, plus the ok/fail/isOk/isFail paths and wrong-variant TypeErrors. It also includes a parity check asserting the instance exposes exactly the r-result method set, so future refactors stay protected by CI.

npm test now runs both suites: 85 passing (51 existing + 34 new). No behavior change — irc-socket.js simply requires ./result.js.

@silverbucket
silverbucket merged commit 1b479af into master Jul 4, 2026
6 checks passed
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