-
Notifications
You must be signed in to change notification settings - Fork 141
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🚚 move recorder modules #846
Conversation
@@ -63,7 +62,7 @@ describe('recorder', () => { | |||
) | |||
}) | |||
|
|||
describe('snapshot', () => { | |||
describe('full snapshot', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is full snapshot equivalent to document serialization?
if yes, what about renaming these references as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A full snapshot is a FullSnapshot record, which contain the serialized document and the document scroll offset. I think we should keep the same naming to keep consistency between player and recorder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not hesitate to tweak the naming on player and recorder side if it makes more sense to us.
You have a better grasp on that so I'll let you decide what is making the more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would agree that we could improve the protocol namings, but I would not do this incrementally by renaming only FullSnapshot and keeping IncrementalSnapshot for example. I think it would be worth to discuss more about this with an RFC when we feel the need for it.
Codecov Report
@@ Coverage Diff @@
## main #846 +/- ##
==========================================
- Coverage 88.13% 88.05% -0.09%
==========================================
Files 81 80 -1
Lines 3666 3666
Branches 821 821
==========================================
- Hits 3231 3228 -3
- Misses 435 438 +3
Continue to review full report at Codecov.
|
Motivation
As a follow up of #842 , I wanted to move "recorder" source files a bit to have a clearer module hierarchy.
Changes
Proposed new hierarchy (not showing spec files):
Testing
CI
I have gone over the contributing documentation.