Break apart plugin reconciler #332
Conversation
| expect(size(patch.added)).to.equal(1) | ||
| expect(patch.added["CHILD"]).to.equal(virtualInstances["CHILD"]) | ||
| end) | ||
| end |
There was a problem hiding this comment.
This needs a test case to ensure the diff scans the entire virtual DOM/instance map. Currently this only tests the case where the root has an immediate child but doesn't check if changes to that child (and it's children) get included in the diff.
| expect(size(patch.added)).to.equal(1) | ||
| expect(patch.added["CHILD"]).to.equal(virtualInstances["CHILD"]) | ||
| end) | ||
| end |
There was a problem hiding this comment.
An alternative test case where you have a cyclic reference may also be good. I doubt this can come up during normal use however it would be nice to ensure it doesn't blow up rojo in the event it does somehow happen.
There was a problem hiding this comment.
I'm not too worried about this case coming up. I think we can avoid adding the extra machinery to detect cycles unless it's something that could feasibly happen.
| return true | ||
| end | ||
|
|
||
| return setProperty |
There was a problem hiding this comment.
getProperty and setProperty both share a lot of functionality, is there a way we could have more code reuse between them? I'm not sure if it's the best way but I tackled a similar problem by making a function like propertyOperation("Write", instance, propertyName, value). What do you think about this kind of approach?
There was a problem hiding this comment.
I think the similarities are pretty much just the error handling boilerplate. Maybe cutting down on that would make these functions feel less similar?
|
Addressed and replied to most of the review feedback. I'll add more test cases tomorrow and see if I can get a baseline |
This was exposed trying to sync the Rojo plugin, which has a gigantic ModuleScript in it with the reflection database. This workaround was present in some form in many versions of Rojo, and I guess we still need it. This time, I actually documented why it's here so that I don't forget for the umpteenth time...
|
Let's go forward with this. I think this is stable enough to promote to master, and it's a pretty big improvement in many ways. |
* Start splitting apart reconciler, with tests * Reify children in reify * Baseline hydrate implementation * Remove debug print * Scaffold out diff implementation, just supporting name changes * invariant -> error in decodeValue * Flesh out diff and add getProperty * Clear out top-level reconciler interface, start updating code that touches it * Address review feedback * Add (experimental) Selene configuration * Add emptiness checks to PatchSet, remove unimplement invert method * Improve descendant destruction behavior in InstanceMap * Track instanceId on all reify errors * Base implementation of applyPatch, returning partial patches on failure * Change reify to accept InstanceMap and insert instances into it * Start testing applyPatch for removals * Add test for applyPatch adding instances successfully and not * Add , which is just error with formatting * Correctly use new diff and applyPatch APIs * Improve applyPatch logging and fix field name typo * Better debug output when reify fails * Print out unapplied patch in debug mode * Don't write properties if their values are not different. This was exposed trying to sync the Rojo plugin, which has a gigantic ModuleScript in it with the reflection database. This workaround was present in some form in many versions of Rojo, and I guess we still need it. This time, I actually documented why it's here so that I don't forget for the umpteenth time... * Add placeholder test that needs to happen still * Introduce easier plugin testing, write applyPatch properties test * Delete legacy get/setCanonicalProperty files * Fix trying to remove numbers instead of instances * Change applyPatch to return partial patches instead of binary success * Work towards being able to decode and apply refs * Add helpers for PatchSet assertions * Apply refs in reify, test all cases * Improve diagnostics when patches fail to apply * Stop logging when destroying untracked instances, it's ok * Remove read before setting property in applyPatch * Fix diff thinking all properties are changed
The current reconciler is monolithic and hard to test. This PR breaks apart Rojo's reconciler into smaller pieces that are easier to test in isolation.
Design
On initial sync, the Rojo plugin goes through this process:
hydratethe real DOM using IDs from the virtual DOMdiffthe real DOM and the virtual DOM, generating a patchapplyPatchthe patch to actually make these changesEvery time the server notifies Rojo of a change, the Rojo plugin goes through this process:
applyPatchthe patch sent from the server. That's it!Almost all of these methods are fallible. They'll usually fail due to permission errors, but might also run into unknown or unexpected data that can't be predicted beforehand.
Hydration -
hydrateHydration is the process of collecting all existing instances, matching them up to virtual instances that the server knows about, and assigning them IDs. This process populates a type known as
InstanceMap, which is a bidirectional map between Rojo's server-generated IDs and actual instances in Roblox Studio.Anything that doesn't have an ID at this point is not present on the server and should potentially be deleted by the plugin.
Diffing -
diffThis step calculates a patch that would catch up the real DOM to match the virtual DOM. It depends on the DOM being hydrated, which drastically simplifies this process. Each property is measured and compared with the virtual equivalent, unknown virtual instances are marked for addition in the patch, and any unknown real instances are marked for deletion.
By returning a patch here instead of directly applying changes, I'm hoping we can use this for two way sync. On initial sync, we'll detect that there's some drift and let the user pick which side's changes should be kept. We'll need a way to invert a patch, which is fairly difficult, but this is a powerful technique.
Patch Application -
applyPatchThis is the only actual side-effectful function. This function takes a patch and applies it to the real DOM.
applyPatchwill try to apply as much of a patch as it can, and return any parts that couldn't be applied. This is different than what Rojo does on the master branch currently! The goal of this behavior is to be able to fall back to other approaches for instances that couldn't be created or updated from Lua by normal means. The big one the trick outlined in #205, but any other form of content negotiation with the server could happen here.applyPatchwill sometimes call into another function that isn't exposed in the top-level API,reify.reifywill attempt to create an instance, set all properties on it, and create all of its descendants. If an instance can't be created or a property can't be set,applyPatchwill do as much as it can, and return a partial patch containing what couldn't happen.TODO
reifydiffshould not pick up properties that didn't actually change, but it might be.