-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
add Snapshot#serialize method #3148
Conversation
yratanov
commented
Jun 2, 2015
- fixes Implement snapshot.serialize() #3144
@return {Object} an object whose values are primitive JSON values only | ||
*/ | ||
serialize: function(options) { | ||
return this.record.store.serializerFor(this.modelName). |
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.
you can just say this.record.serialize() and pass options
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 thought snapshot.serialize should serialize data from snapshot, not from underlying record. See my test below it won't pass if I do as you wrote.
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.
good point.
Can you then make store.serialize use snapshot.serialize
instead of repeating code?
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.
@igorT done.
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.
Can you please make this 1 line, I don't think we do this for lines this short
Can you please squash, and I'll merge |
|
||
post.set('title', 'New Title'); | ||
|
||
deepEqual(snapshot.serialize(), { author: undefined, title: 'Hello World' }, 'shapshot serializes correctly') |
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.
Could we add an additional assert and pass options
?
* fixes #3144 use Snapshot#serialize in Store#serialize
PS: where can I see your style guides? except this https://github.com/emberjs/data/blob/master/CONTRIBUTING.md |
LGTM 👍 @yratanov There's a style guide at https://github.com/emberjs/ember.js/blob/master/STYLEGUIDE.md |
Thanks! |