Mark public ObjectMessage.id and .timestamp as optional#2230
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughObjectMessage type marks ChangesObjectMessage field optionality
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
These declared types are incorrect since these values are not populated for ObjectMessages created by apply-on-ACK. This is a breaking change to the type declarations, but I think it's the right thing to do given that the declarations do not match the runtime behaviour. You could argue that we could try and fix the runtime behaviour; i.e backfill `timestamp` based on the server time or something, but I'm not sure it'd be easy to backfill `id`. Thoughts welcome. Noticed this whilst working on the spec for the path-based API in [1]; see PAOM2a/d there. I've incorporated the API change from the current commit into those points. [1] ably/specification#427 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
02ac44b to
7552659
Compare
These declared types are incorrect since these values are not populated for
ObjectMessages created by apply-on-ACK.This is a breaking change to the type declarations, but I think it's the right thing to do given that the declarations do not match the runtime behaviour. You could argue that we could try and fix the runtime behaviour; i.e backfill
timestampbased on the server time or something, but I'm not sure it'd be easy to backfillid. Thoughts welcome.Noticed this whilst working on the spec for the path-based API in ably/specification#427; see PAOM2a/d there. I've incorporated the API change from the current commit into those points.
Summary by CodeRabbit