-
-
Notifications
You must be signed in to change notification settings - Fork 27
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
Fix snapshot/restore methods to separate keys and values #167
base: main
Are you sure you want to change the base?
Fix snapshot/restore methods to separate keys and values #167
Conversation
@sapryniukt Thanks for the PR! Is this structure useful for you? If you try and access |
@snewcomer Yep! As I understand it, |
Do you have a test case that shows a bug that was fixed? This would be a breaking change and still unsure of the value. Happy to consider it though! |
test/index.test.ts
Outdated
@@ -3256,25 +3256,31 @@ describe('Unit | Utility | changeset', () => { | |||
}); | |||
}); | |||
|
|||
it('#restore restores a snapshot of the changeset with nested values', () => { | |||
let user = { name: 'Adam', address: { country: 'United States' } }; | |||
it('#restore restores a snapshot of the changeset when nested value is object', () => { |
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.
@sapryniukt could you please keep the existing test case as is and only add a new test?
cc @snewcomer this test mimics the scenario that fails as today restore
would not honor an instance of the Country
class and would only restore the object structure
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.
Sorry for my lack of familiarity but I’m guessing we can’t fix the bug by keeping the existing data shape?
6361e4c
to
1ec241d
Compare
1ec241d
to
21637d2
Compare
this PR is based on #24 issue and #27 pull request.
After updating the key storage template from string to nested keys, especially in the
snapshot()
andrestore()
methods there is no clear separation of keys and values of changes if both are objects.snapshot()
returns structure likeThe idea is not to use
getChangeValue()
instead,It will have a structure like
Update 04 August 2022
A simpler solution was applied to avoid changing the shape of data and changes could keep their own proptotypes. In
reduce
method of#restore
we should setinitialValue
as: