Details
Details
NA, flow
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
This looks good, but the use of the type spread operator breaks the read-only-ness of these types. This wasn't something I was aware of at the time the types were introduced. Would you mind making them read-only in this diff?
| lib/types/socket-types.js | ||
|---|---|---|
| 201–205 ↗ | (On Diff #35553) | Wrap in $ReadOnly |
| 206–210 ↗ | (On Diff #35553) | Wrap in $ReadOnly |
| 211–216 ↗ | (On Diff #35553) | Wrap in $ReadOnly |
| 218–222 ↗ | (On Diff #35553) | Wrap in $ReadOnly |
| 229–234 ↗ | (On Diff #35553) | Wrap in $ReadOnly |
Comment Actions
Ah, missed the plan changes – feel free to re-request review later if there are any significant changes
| lib/types/socket-types.js | ||
|---|---|---|
| 229 ↗ | (On Diff #35587) | Get the following when I try making this $ReadOnly:  |
Comment Actions
Going to land as is, created a task to track making ServerStateSyncFullSocketPayload ReadOnly: https://linear.app/comm/issue/ENG-6515/make-serverstatesyncfullsocketpayload-readonly
Worth noting that ServerStateSyncFullSocketPayload was not part of this diff.