Enhance reliability of obtaining colletion from interval #16251
Conversation
github-actions
bot
added
area: dds
⯅ @fluid-example/bundle-size-tests: +491 Bytes
Baseline commit: 695bb1d |
| /** | ||
| * {@inheritDoc ISerializableInterval.getRangeLabels} | ||
| */ | ||
| public getRangeLabels(): string[] { |
There was a problem hiding this comment.
I don't think this PR needs to change the public API of intervals. It looks like we already store the collection label name as an interval prop and correctly strip this at serialization time + populate it at deserialization time. The work item linked is more about making sure application code doesn't modify this value as a call to changeProperties (which it looks like you do have that logic). We should still be able to retrieve it using the same logic we did before.
There was a problem hiding this comment.
I suppose the collection label and interval id have some similarities as they are both stored as interval props reservedRangeLabelKeys and reservedIntervalIdKey, and we do not want them to be modified explicitly. So I suppose it mihgt be a better practice to implement an API having similar purpose with getIntervalId()? Not sure if the benefit of adding it can outweigh the potential drawbacks
There was a problem hiding this comment.
getIntervalId is also a useful API that applications need to call though--this one it's not clear that we have any external users that want it, so I'd rather avoid cluttering the API surface unless we get specific, reasonable feedback that it's necessary for some scenario.
| // This check is intended to prevent scenarios where a random interval is created and then | ||
| // inserted into a collection. The aim is to ensure that the collection is created first | ||
| // then the user can create/add intervals based on the collection | ||
| if ( |
There was a problem hiding this comment.
nit: I don't think our error message here needs to be particularly friendly. asserting here and below might give better bundle size. not a big deal though.
Description
AB#4524, AB#4081
This PR implements enhancements to improve the reliability of obtaining interval collections from intervals. Following discussions with @scarlettjlee, we concluded that directly making the properties private is not feasible since we need to ensure the existing data at rest continue to work. As a result, several compromise strategies are included in this PR:
Each interval should only ever be in one collection, their labels should be consistent
We encourage users to create/add an interval based on existing collection, rather than creating random interval and insert them into collections
2. Add a new API toISerializableIntervalto get the labels, allowing users to retrieve the labels similar togetIntervalId(). We discourage users to get access to the properties directly, despite they are public.