Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Animated Value persist its value after reattaching #87

Merged
merged 16 commits into from
Sep 28, 2018
Merged

Conversation

osdnk
Copy link
Contributor

@osdnk osdnk commented Aug 31, 2018

When value gets detached from all component it can be still kept in memory and attached to some other component at later time. But because we don't know when JS reference is dropped we need to delete native counterpart of the value when it gets detached. This has led to a situation in which the value was reinstantiated on the native site. In that case if the value has changed we expect it to persist that changes when it gets attached again. This wasn't happening and each time the value was instantiated we'd always use the initial value.

With this change we are solving that issue by querying the value right before we detach the native node and then we use that value to update config such that next time when it gets instantiated it uses updated value instead of the initial one.

@osdnk osdnk changed the title init Make Animated Value persist its value after reattaching Aug 31, 2018
@osdnk
Copy link
Contributor Author

osdnk commented Aug 31, 2018

related: #85

@osdnk osdnk added WIP and removed WIP labels Aug 31, 2018
@@ -94,7 +94,7 @@ export default class AnimatedNode {

__nativeInitialize() {
if (!INITIALIZED_NODES.has(this.__nodeID)) {
ReanimatedModule.createNode(this.__nodeID, this.__nodeConfig);
ReanimatedModule.createNode(this.__nodeID, { ...this.__nodeConfig });
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this destruction in order not to fleeze value field

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good but I think it would be easier to use callback to get the value instead of introducing new event type. That is have a method:

ReanimatedModule.getValue(this.__nodeID, v => {
   this.__nodeConfig.value = v;
});

@osdnk
Copy link
Contributor Author

osdnk commented Sep 10, 2018

@kmagiera
Now I am not convinced at all about this change. I'm afraid of blinking with some edge cases like this one which @ericvicenti has shown

@ckknight
Copy link
Contributor

@kmagiera Why use a callback instead of returning a promise?

@kmagiera
Copy link
Member

@osdnk looks like the blinking problem wasn't related to the problem

@ckknight we can also use promise instead of callback, what I wanted to get rid is that special type of event that we expect to be sent when we call a method. Much cleaner pattern would be to call getValue method and expect it to return its value asynchronously (e.g. via callback or a promise) instead of complicating its contract and expecting special even to be returned.

Copy link
Member

@kmagiera kmagiera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, can you think of an example we can add to test if this continue to work in the future

@@ -3,6 +3,8 @@ import { set } from '../base';
import { val } from '../utils';
import { evaluateOnce } from '../derived/evaluateOnce';
import interpolate from '../derived/interpolate';
import ReanimatedModule from '../ReanimatedModule';
import ReanimatedEventEmitter from '../ReanimatedEventEmitter';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

import cleanup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants