Skip to content

Update ObjectData value properties spec to match latest implementation#327

Merged
VeskeR merged 1 commit intomainfrom
update-object-data
Jun 10, 2025
Merged

Update ObjectData value properties spec to match latest implementation#327
VeskeR merged 1 commit intomainfrom
update-object-data

Conversation

@VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Jun 9, 2025

This was changed in ably-js PR [1]

[1] ably/ably-js#2026

** @(OD2b)@ @encoding@ string - the encoding the client library should use to interpret the @value@
** @(OD2c)@ @value@ string, number, boolean or binary - a concrete value of the object
** @(OD2b)@ @encoding@ string - can be set by the client to indicate that value in @string@ or @bytes@ field have an encoding
** @(OD2c)@ This clause has been deleted
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there was a discussion recently about how strictly we should adhere to the rules regarding deleting/updating spec points in order to minimaze polluting the spec with "This clause has been deleted" statements. @lawrence-forooghian
Do you think it makes sense to add this statement now in this case and for OD3a? The development in other SDKs just started but I assume you may already have references to those spec points

Copy link
Collaborator

@sacOO7 sacOO7 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should make a interface/class for ObjectValue, that way it's easy to interpret, instead of 4 top level fields for value.

Copy link
Collaborator

@sacOO7 sacOO7 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export class ObjectValue {
  public value: boolean | Bufferlike | number | string;

  constructor(value: boolean | Bufferlike | number | string) {
     // runtime type checks can be added if used as a part of public API
     this.value = value
  }

  getSize(): number {
    if (typeof this.value === 'boolean') {
      return 1;
    }
    if (typeof this.value === 'number') {
      return 8;
    }
    if (typeof this.value === 'string') {
      return this.value.length;
    }
    // bytes (Bufferlike)
    if (this.value && typeof this.value === 'object' && 'byteLength' in this.value) {
      return (this.value as any).byteLength;
    }
    if (this.value && typeof this.value === 'object' && 'length' in this.value) {
      return (this.value as any).length;
    }
    return 0;
  }
}

Copy link
Collaborator

@sacOO7 sacOO7 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So ObjectData becomes

export interface ObjectData {
  /** A reference to another object, used to support composable object structures. */
  objectId?: string;

  /** Can be set by the client to indicate that value in `string` or `bytes` field have an encoding. */
  encoding?: string;
  
  /** The value stored in this ObjectData. */
  value?: ObjectValue;
}

This way it's easy to read with better checks, and we encapsulate business logic as a part of ObjectValue

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other languages, we can have four different constructors that initializes ObjectValue with given type.

Copy link
Collaborator

@sacOO7 sacOO7 Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy for it to be replaced ( also true for subsequent specs ), since it's not implemented in any other SDKs yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an ably-cocoa point of view, implementing these spec points is still a WIP so am happy for them just to be replaced, will leave Sachin to decide.

I'd prefer spec points to be replaced without the "This clause has been deleted" statement to keep it cleaner while we still can and it's WIP.
So @sacOO7 it's your call since you may have some code already written for this - can I just replace the spec for the value property and not include the "This clause has been deleted"?

Copy link
Contributor Author

@VeskeR VeskeR Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy for it to be replaced ( also true for subsequent specs ), since it's not implemented in any other SDKs yet

oh, I see, thanks! You replied while I was writing a comment so I didn't see it

Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian Jun 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sachin said above that he's happy for it to be replaced

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will update the PR and merge it

Copy link
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@VeskeR VeskeR force-pushed the update-object-data branch from 7f845c3 to c2f155a Compare June 10, 2025 08:38
@VeskeR VeskeR merged commit 1119079 into main Jun 10, 2025
2 checks passed
@VeskeR VeskeR deleted the update-object-data branch June 10, 2025 08:38
@lawrence-forooghian lawrence-forooghian added the live-objects Related to LiveObjects functionality. label Jun 12, 2025
VeskeR added a commit to ably/ably-js that referenced this pull request Jun 20, 2025
…Message class

Based on the spec added in [1], and updates to the spec made in [2] and [3]

[1] ably/specification#298
[2] ably/specification#327
[3] ably/specification#328
VeskeR added a commit to ably/ably-js that referenced this pull request Jun 26, 2025
…Message class

Based on the spec added in [1], and updates to the spec made in [2] and [3]

[1] ably/specification#298
[2] ably/specification#327
[3] ably/specification#328
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

live-objects Related to LiveObjects functionality.

Development

Successfully merging this pull request may close these issues.

3 participants