Improve docstring for channels#release and add log

[SimonWoolf]

ably/specification#448

The current docstring almost makes it sound like we expect people to call release(). 99% of people should not. There's a reason the original ably-js comment on this method was /* Included to support certain niche use-cases; most users should ignore this. Please do not use this unless you know what you're doing */

Summary by CodeRabbit

• Documentation
  • Clarified channel "release" behavior, including warnings about using channel references after release and updated state preconditions.
• Bug Fixes
  • Releasing a channel now ensures channels in non-terminal states are detached before being released, and release operations are logged and completed asynchronously to avoid leaving dangling references.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9dbd88c3-bb91-43ea-8ec6-9c061af10f4b

📥 Commits

Reviewing files that changed from the base of the PR and between c30309c and 861bdc7.

📒 Files selected for processing (2)

• ably.d.ts
• src/common/lib/client/baserealtime.ts

---

Walkthrough

Channels.release() JSDoc clarified to describe releasing SDK-held references (not deleting channels); implementation now logs the release action and conditionally detaches realtime channels before removing internal references, deleting the channel entry after detach completes or immediately when already in terminal/initialized states.

Changes

Cohort / File(s)	Summary
TypeScript Definitions ably.d.ts	Updated Channels<T>.release(name: string) JSDoc to state it releases SDK-held references (not deleting), warns about undefined behavior when using released references, and adjusts channel-state precondition wording.
Channels release implementation src/common/lib/client/baserealtime.ts	Added Logger.logAction call at release start; changed control flow to delete this.all[name] immediately only when channel state is INITIALIZED, DETACHED, or FAILED; otherwise call channel.detach() asynchronously, log detach errors, and delete the channel entry after detach resolves.

Sequence Diagram(s)

sequenceDiagram
  participant Caller as Caller
  participant Channels as Channels Manager
  participant Logger as Logger
  participant Channel as Channel Instance

  Caller->>Channels: release(name)
  Channels->>Logger: logAction("releasing references", name)
  Channels->>Channel: getState(name)
  alt state is INITIALIZED / DETACHED / FAILED
    Channels->>Channels: delete this.all[name]
    Channels-->>Caller: resolve
  else other states
    Channels->>Channel: detach() (async)
    Note over Channel: detach may succeed or fail
    Channel-->>Channels: detach resolved / rejected
    alt resolved
      Channels->>Channels: delete this.all[name]
      Channels-->>Caller: resolve
    else rejected
      Channels->>Logger: logAction("detach failed", error)
      Channels->>Channels: delete this.all[name]
      Channels-->>Caller: resolve
    end
  end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I nudge the docs and whisper, "Free,"
Channels unbound, let references flee,
A log, a detach, then softly gone —
I thump my foot and greet the dawn! 🥕✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: improving the docstring for channels#release and adding a log message.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch release-warning

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[SimonWoolf]

....I've just looked through and found ably/docs#1057 (comment) where apparently we decided to change the spec to make release() implicitly detach first, but then didn't do that in ably-js? bleh. So put this on hold till we figure out what to do with that

[SimonWoolf]

I've switched the implementation to an RTS4a-compliant one, ie implicit detach rather than error on unexpected channel state.

(honestly I would prefer an error, this is a dangerous method and I think making it automagic is a step in the wrong direction, but all other SDKs implement the other behaviour and can't be changed to the ably-js semantics without a breaking change, so, shrug, this is the simplest way to uniformity)

[AndyTWF]

Does this necessitate a major version bump as we're changing the behaviour? Is that the plan?

[SimonWoolf]

It's a semantics change, but in the direction of being strictly more permissive, which makes it a backwards-compatible change. If someone who was using the function before without it throwing would continue to work. (and note how it would be naturally async now but I did the slightly-awkward .catch().then() so that I'm not changing the signature of the function to return a promise).

This is why I went this route rather than changing ably-java etc to be stricter, which I would have preferred, but that would require a major version bump.

(in some sense, any observable behaviour change could be relied on by someone: it's not literally impossible that someone is relying on the behaviour that release() throws for an attached channel. but it's pretty damn unlikely, and an interpretation of semver where such a change would require a major version bump would imply there can be no such thing as a minor or patch bump. every code change other than pure refactor causes some observable behaviour change or there would be no point doing it, anyone can rely on anything. so ultimately it has to be a judgement over what people are likely to be relying on. cf https://xkcd.com/1172/ )