Add ability to diff/patch states. Closes #36 #37
Conversation
src/common/JSONImporter.js
Outdated
| await this.applyDiffs(diffs, resolvedSelectors); | ||
| } | ||
|
|
||
| async getDiffs(node, state, resolvedSelectors=new NodeSelections()) { |
There was a problem hiding this comment.
I would just call this diff
There was a problem hiding this comment.
Since we have changesets.js already named diff. I am not sure, do we want to rename it to not create any confusion?
src/common/JSONImporter.js
Outdated
| return diffs; | ||
| } | ||
|
|
||
| async applyDiffsForNode(node, diffs) { |
There was a problem hiding this comment.
I would rename this to patch or patchNode. Maybe make the node argument optional and name it patch?
src/common/JSONImporter.js
Outdated
| await this.applyDiffs(diffs, resolvedSelectors); | ||
| } | ||
|
|
||
| async applyDiffs(diffs, resolvedSelectors) { |
There was a problem hiding this comment.
I would name this patch. It would be nice if we had patch and patchSync where the latter required resolvedSelectors to already contain all selectors used in the patch/changeset.
There was a problem hiding this comment.
However, this won't work if we want to support the lazy creation of subtrees (in which it diffs the subtree when the node is created).
| await this._put(created, change, resolvedSelectors); | ||
| })); | ||
| await Promise.all((state.children || []).map(async child => { | ||
| await this.createStateSubTree(this.core.getPath(created), child, resolvedSelectors); |
There was a problem hiding this comment.
I think this might have problems with inherited children. I will put together a test and check.
There was a problem hiding this comment.
It looks like this is a problem in main, too, so probably not a deal breaker...
Currently, the resolveSelectors, doesn't record the node if the state doesn't have ID. However, in `patchSync` we might need the node info even if the state id is not present. This refactor tries to handle it by adding the current node's path to cache as well.
src/common/JSONImporter.js
Outdated
| } | ||
|
|
||
| async applyDiffs(diffs, resolvedSelectors) { | ||
| async patchSync(diffs, resolvedSelectors) { |
There was a problem hiding this comment.
I was thinking that patchSync would be synchronous :)
There was a problem hiding this comment.
Maybe leave it async (named _patch or something and save the synchronous version for another PR.
There was a problem hiding this comment.
I think this should be good to merge. One minor thing is that it might be nice to try to more closely match the format of the patches for add_subtree and delete_subtree. For example:
{
"type": "put",
"key": ["children"],
"value": childState
}
// and
{
"type": "del",
"key": ["children"],
"value": childPath
}
|
Looks great! Thanks, @umesh-timalsina! |
- Currently, nodes are compared according to their Ids. One of the things that makes a criss-cross comparison a bit difficult is because, we might not have access to the
coreapi to find/search nodes while just diffing JSONs in one pass (Quicker and Synchronous). And applying only necessary changes in the next pass (Slower and Asynchronous) but only to the parts of the subtree with actual changes.@idresolution