fix for references to objects being broken after replacing the node #1053
Conversation
|
Does you completely disable optimization with "object" mode in StoredReference? |
|
it wasn't much of an optimization given there is an identifier cache underneath that keeps a hash map with a pointer to the node. |
|
Also, at first I tried to keep it, but apparently making a mutation (in this case of the ref object) in the middle of a read operation (getValue) makes the system go nuts |
|
Ok, I added back resolve optimizations, but in a better way since now it even works for refs that use ids, not only for refs that use objects |
| this.targetType.name | ||
| }' (from node: ${node.path})` | ||
| ) | ||
| this.resolvedNode = { |
There was a problem hiding this comment.
I'd call it resolvedReference or just resolved, becuase this.resolvedNode!.node later on looks quite strange :)
| lastCacheModification: string | ||
| } | ||
|
|
||
| constructor(mode: "identifier" | "object", value: any, private readonly targetType: IAnyType) { |
There was a problem hiding this comment.
I might have missed something, but it seems like we do not need mode at all for now. In constructor we can do if (isStateTreeNode(value)) instead of mode === "object" (becuase mode is chosen by that criteria at instantinate).
During reconcilation we could do:
const compareByValue= isStateTreeNode(newValue)
const ref = current.storedValue as StoredReference
if (!compareByValue && ref.identifier === newValue) return current
else if (compareByValue && ref.resolvedValue === newValue) return current
...
|
@k-g-a thanks for the review! all requested changes done :) |
|
Feel free to release :-) |
|
minor or patch? asking because it is a feature-bugfix |
|
I think given the docs one would already assume this to work, so patch
should be ok?
Op di 23 okt. 2018 om 13:27 schreef Javier Gonzalez <
notifications@github.com>:
… Merged #1053 <#1053> into
master.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#1053 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABvGhNvDmK4cObVpQghUhYVTJmeUkYaoks5unv0YgaJpZM4XyHVW>
.
|
Fixes #1052