Skip to content

Commit

Permalink
Fix node dirty marking when new connection between nodes is made (#450)
Browse files Browse the repository at this point in the history
This change fixes the problem where a parent node were marked as dirty while adding connection between nodes instead of the children node. We should be only marking children node as if we mark parent node there is a chance that this would trigger updates in leave nodes (final nodes) that should not be affected as the children added to parent node cannot impact its value.

On top of that the marking change relealed a bug where we initialized loop ID inproperly. We should be initializing it with a value that'd allow the node to be evaluated at least once. It's been working before as every node would be dirty marked. With the above change the root nodes were no longer being marked but we'd still require them to be evaluated. The invalid initial setting for last loop ID was making us skip evaluation of those parent nodes.

In order to test this change I created the following sample app: https://gist.github.com/7fce6b4a891f0284baa17d0d63455495
In this app there is a button that creates a new view that hooks into an existing animated value. Before this change when the view was added we'd reevaluate all the nodes that depend on that value and hence we'd get a consol.warn being shown twice (first on the initial render and then second time when we add the view). WIth this change in the `call` node should not be evaluated more than once and only on the initial render.
  • Loading branch information
kmagiera authored Oct 31, 2019
1 parent 73f8e3f commit 24be3e3
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public abstract class Node {
private @Nullable List<Node> mChildren; /* lazy-initialized when a child is added */

public Node(int nodeID, @Nullable ReadableMap config, NodesManager nodesManager) {
mLastLoopID.put("", 1L);
mLastLoopID.put("", -1L);
mNodeID = nodeID;
mNodesManager = nodesManager;
mUpdateContext = nodesManager.updateContext;
Expand Down Expand Up @@ -75,7 +75,7 @@ public void addChild(Node child) {
mChildren = new ArrayList<>();
}
mChildren.add(child);
dangerouslyRescheduleEvaluate();
child.dangerouslyRescheduleEvaluate();
}

public void removeChild(Node child) {
Expand Down
4 changes: 2 additions & 2 deletions ios/Nodes/REANode.m
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ - (instancetype)initWithID:(REANodeID)nodeID config:(NSDictionary<NSString *,id>
_nodeID = nodeID;
_lastLoopID = [NSMutableDictionary dictionary];
_memoizedValue = [NSMutableDictionary dictionary];
_lastLoopID[@""] = @1;
_lastLoopID[@""] = 0;
}
return self;
}
Expand Down Expand Up @@ -86,7 +86,7 @@ - (void)addChild:(REANode *)child
}
if (child) {
[_childNodes addObject:child];
[self dangerouslyRescheduleEvaluate];
[child dangerouslyRescheduleEvaluate];
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/core/AnimatedSet.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@ import AnimatedNode from './AnimatedNode';
import { val } from '../val';
import { adapt } from '../core/AnimatedBlock';

import invariant from 'fbjs/lib/invariant';

class AnimatedSet extends AnimatedNode {
_what;
_value;

constructor(what, value) {
super({ type: 'set', what: what.__nodeID, value: value.__nodeID }, [value]);
invariant(!what._constant, 'Value to be set cannot be constant');
this._what = what;
this._value = value;
}
Expand Down

0 comments on commit 24be3e3

Please sign in to comment.