This repository has been archived by the owner on Jun 26, 2020. It is now read-only.
Internal: Changed the way of data serialization. #1493
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pjasiun
reviewed
Aug 10, 2018
src/model/position.js
Outdated
| @@ -915,7 +926,7 @@ export default class Position { | |||
| */ | |||
| static fromJSON( json, doc ) { | |||
| if ( json.root === '$graveyard' ) { | |||
| const pos = new Position( doc.graveyard, json.path ); | |||
| const pos = new Position( doc.graveyard, Array.from( json.path ) ); | |||
There was a problem hiding this comment.
This will be removed.
pjasiun
reviewed
Aug 10, 2018
| result[ attr[ 0 ] ] = attr[ 1 ]; | ||
|
|
||
| return result; | ||
| }, {} ); |
pjasiun
pushed a commit
that referenced
this pull request
Aug 14, 2018
Other: Make `toJSON()` to serialize nested objects. Closes #1477.
scofalik
reviewed
Aug 16, 2018
| /** | ||
| * @inheritDoc | ||
| */ | ||
| toJSON() { |
There was a problem hiding this comment.
I wonder here... Theoretically, values could be arrays or objects. Will those be cloned?
There was a problem hiding this comment.
BTW... maybe we should just give up that and allow only for string, numeral and boolean values for attributes?
There was a problem hiding this comment.
🤔I'm afraid in a case of objects we pass a reference to the value instead of cloning.
We can think about dropping other formats than primitives but we don't know if someone has already used it with objects or arrays.
scofalik
reviewed
Aug 16, 2018
|
|
||
| json.position = this.position.toJSON(); | ||
|
|
||
| if ( json.graveyardPosition ) { |
There was a problem hiding this comment.
Wrap operation can have graveyard position or element. Shouldn't we do json.element = this.element.toJSON() ?
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggested merge commit message (convention)
Internal: Changed the way of data serialization. Closes ckeditor/ckeditor5#4392.
Additional information
For example – encountered issues, assumptions you had to make, other affected tickets, etc.