Change naming of APIs for state and fetching state

[lawrence-forooghian]

https://github.com/ably-labs/ably-chat-swift/pull/54#discussion_r1782660210

@umair-ably wasn’t a fan of:

• using current as a property to mean "the current state of this object", and would prefer it be named state
  • Note: the naming of current is used in the public API of the SDK, which matches the JS naming and also matches the naming used in the core SDKs, and personally whilst I think it could be clearer ("current" what?), I can also live with it
• the naming of the RoomLifecycle enum, which he thinks should be called RoomState or RoomLifecycleState
  • Again, this naming comes from JS
  • I agree with this suggsted change but would like to understand the motivation for the current name, ask @AndyTWF

See comment for agreed outcome to resolve this issue.

┆Issue is synchronized with this Jira Task by Unito

[AndyTWF]

using current as a property to mean "the current state of this object", and would prefer it be named state

We went with current for parity with the core SDKs.

We specifically didn't choose state because state often implies a larger, internal state. status on the other hand implies a single value, e.g. "the status of your order: accepted, delivered". We felt in context, it worked well, e.g.:

  room.status.current

In this example it's quite clear that you're getting the current room status.

the naming of the RoomLifecycle enum, which he thinks should be called RoomState or RoomLifecycleState

Initially we wanted to call these enums RoomStatus and ConnectionStatus, but JS's lack of namespaces and conflicting identifiers made it not possible.

We wanted to avoid the term State for reasons mentioned above.

If you feel there's a better name, then happy to discuss alternatives!

[vladvelici]

@AndyTWF I just had a look at our JS code. I think if we swap the names everything makes more sense.

	Current	New
The enum of statuses	RoomLifecycle	RoomStatus
The object you use to get the status or subscribe to changes	RoomStatus	RoomLifecycle

[umair-ably]

I agree with context to room room.status.current makes sense but when looking at the lifecycle manager it becomes a bit confusing e.g. RoomLifeCycleManager.current?

I'm also fine with status over state, it's the current being a RoomLifecycle enum that doesn't feel right.

Do we need to achieve parity with the core SDKs? or was this something that felt right but perhaps doesn't matter as long as we try to keep parity between the Chat SDKs?

[AndyTWF]

Do we need to achieve parity with the core SDKs? or was this something that felt right but perhaps doesn't matter as long as we try to keep parity between the Chat SDKs?

It was a deliberate design decision - we're expecting the Chat SDK to be used in concert with the core SDK in many applications (people using PubSub for other aspects of their app). So we went for parity whenever there's overlapping concepts between the two so that that things are intuitive between SDKs.

We did deviate in one or two places where it was an obvious improvement, however (e.g. the subscribe behaviour).

[AndyTWF]

I just had a look at our JS code. I think if we swap the names everything makes more sense.

That's definitely a possible!

[umair-ably]

Ahh I've done some digging... most of my confusion stemmed from the PR this is linked to in which we have a property on the RoomLifecycleManager - private(set) var current: RoomLifecycle.

This is readonly _status: DefaultStatus; in JS which makes a lot more sense, so it seems like we've diverged and this is causing the confusion.

Leaning into @vladvelici's suggestion, the API would make a lot more sense with that change too.

Where we currently have:

class RoomLifecycleManager {
    private(set) var current: RoomLifecycle
}

// impl
lifecycleManager.current = .attached

this would become:

class RoomLifecycleManager {
    private(set) var status: RoomStatus
}

// impl
lifecycleManager.status = .attached

this makes a lot more sense!

Can we confirm we're happy to make the following changes:

• current becomes status in the ably-chat-swift SDK
• RoomLifecycle is renamed to RoomStatus
• RoomStatus is renamed to RoomLifecycle

[paddybyers]

In the core API we only use current when it's a state change event, and that's when there's also a previous. It's not used anywhere to represent the current state outside of that context. current what anyway? If it's current state, then the field should be state.

[AndyTWF]

Can we confirm we're happy to make the following changes:

• current becomes status in the ably-chat-swift SDK
• RoomLifecycle is renamed to RoomStatus
• RoomStatus is renamed to RoomLifecycle

SGTM - for JS we'll swap the interface and enum names for consistency, as well as the current -> status change

[umair-ably]

Sweet, we'll do the same - thanks all! I've also added this to the divergence log

[lawrence-forooghian]

I imagine we want to make the same change for connection lifecycle, right? That is, call the enum ConnectionStatus and the object that emits this status ConnectionLifecycle.

[AndyTWF]

That is, call the enum ConnectionStatus and the object that emits this status ConnectionLifecycle.

For this we already have ConnectionStatus as the enum. I somewhat prefer Connection as the interface name - I don't think Lifecycle on the end adds anything useful / distinguishes it from something else

[lawrence-forooghian]

For this we already have ConnectionStatus as the enum.

Are you sure about that? From what I’m seeing, ConnectionStatus is the interface that emits the enum and ConnectionLifecycle is the enum.

[AndyTWF]

For this we already have ConnectionStatus as the enum.

Are you sure about that? From what I’m seeing, ConnectionStatus is the interface that emits the enum and ConnectionLifecycle is the enum.

Ahh sorry, was looking at the wrong file!

Then yes, I agree we should do the same here :)

[sacOO7]

@AndyTWF Do we need to update this as a part of the spec. I strongly feel, if spec is ambiguous, we need to update it. Because we are going to implement same naming conventions across SDKs

[AndyTWF]

@AndyTWF Do we need to update this as a part of the spec. I strongly feel, if spec is ambiguous, we need to update it. Because we are going to implement same naming conventions across SDKs

The spec for Chat doesn't currently specify down to the property level what everything needs to be called - its a work in progress primarily focused on making sure we capture the intended behaviour in clear language, as well as providing testing points for the UTS.

If we decide to add an IDL to the spec - then of course we'll include it.

[vladvelici]

@AndyTWF wrote CHADR-062 to describe the change, I've left a comment there initially but moving it here to keep the conversation in one place.

---

I think the room.status.status or room.connection.status.status syntax is not ideal, in whatever language.

The room.status.current next to room.status.onChange makes more sense to me, because it’s clear we talk about the status, it’s on a class that represents the status and nothing else. We can subscribe to changes or see the current value.

In JS, inside the implementation of the class, we actually have private variable called _state (could be renamed to _status) and provided as current via a getter. Can similar be done with other implementations to avoid the room.status.status thing?

[sacOO7]

@AndyTWF Do we need to update this as a part of the spec. I strongly feel, if spec is ambiguous, we need to update it. Because we are going to implement same naming conventions across SDKs

The spec for Chat doesn't currently specify down to the property level what everything needs to be called - its a work in progress primarily focused on making sure we capture the intended behaviour in clear language, as well as providing testing points for the UTS.

If we decide to add an IDL to the spec - then of course we'll include it.

I strongly feel we should have IDL for the spec. Otherwise, there will be implicit assumptions in the future that will lead to more confusion

[sacOO7]

Also, it's difficult to keep track of related discussions. I mean current discussion isn't really related to Swift and has impact across other chat SDKs

[lawrence-forooghian]

Conversation moved to https://ably.atlassian.net/wiki/spaces/CHA/pages/3410526209/CHADR-062+Renaming+Lifecycle+Types+for+Clarity