Fixes errorSelector bugs. #611
Conversation
a. errorSelector was not being invoked for branches/replaced nodes during merges. It was only invoked on new leaf insertion. b. errorSelector was being called with truncated paths. This impacted both sets, and merges from the datasourc response. Unit tests coming.
| module.exports = function reconstructPath(currentPath, key) { | ||
|
|
||
| // The first time called during a walk, currentPath.index won't be set. | ||
| var currentPathIndex = (currentPath.index || 0) + 1; |
There was a problem hiding this comment.
The only thing I don't like about this, and we will have to change it at some point, is that set relies an mutating the input with index. We can rely on this for now.
There was a problem hiding this comment.
I agree. I'm happy to feed in index, but it seems like those are heavily coupled already (requestedPath and requestedPath.index always have to by in sync don't they?), so separating them here seems riskier, unless we separate them all the way up.
|
They can be separated, but yes they're always updated in lock-step. Index was added after the recursive function signatures were defined (I didn't realize we needed it till later in the game), so it was purely a cosmetic choice not to go back and update all the signatures (and call-sites) with index params. |
|
Makes sense. Planning to leave them alone for the purposes of this fix/change to get errorSelector working properly. Can treat as a cleanup task. |
Otherwise we'd get the wrong errorPath for [foo, [baz, fizz], bar] and similar iterated key sets of size > 1.
|
Needed to take another stab at it, since due to the nature of the do/while loop, index wasn't enough info to reconstruct the errorPaths fed to the selector for PathSets with keySets of size > 1, e.g. Again, eyeballs appreciated. |
|
@michaelbpaulson - Gonna merge this in, unless you you want to take a look at it? |
|
Sounds great to me!
|
a. errorSelector was not being invoked for branches/replaced nodes
during merges. It was only invoked on new leaf insertion.
b. errorSelector was being called with truncated paths. This impacted
both sets, and merges from the datasource response.
NOTE. This should not be merged yet. Unit tests coming. Wanted to get some eyes on the fix.
@michaelbpaulson