Refactor style and props node evaluation

[simonbuerger]

I'm stepping on some toes here with this PR (@wcandillon, mainly - sorry!)

Related PRs: #311 #131 #176

Related issues: #300

I'm not even sure if this is the most efficient approach (my Java is super rusty), but it seemed that the style node values were unnecessarily being looped over, evaluated, added to a WritableMap, then again in PropsNode they were being looped over, evaluated, type checked and then added to a new WritableMap.

It seemed really inefficient, and I couldn't think of a reason that we wouldn't just merge the style and main props together and only loop and evaluate everything once.

Please shoot me down if this will break things somewhere else.

[wcandillon]

@simonbuerger Huge thanks for being on top of this :)

[simonbuerger]

cc @osdnk @msand

[osdnk]

@kmagiera

[simonbuerger]

For reference I have manually tested this on Android in an animated loan repayment calculator I'm building for a client. It involves animated props that are boolean, string (concatted and formatted currency values passed to TextInput native text prop, and react-native-svg animated path d prop with a few custom bezier curves), and a bunch of style props from width, color, fontSize, opacity and a few translate X and Y transforms. No issues with any of those so far.

[wcandillon]

that sounds so cool :)

…

On Mon 8 Jul 2019 at 11:42, Simon Buerger ***@***.***> wrote: For reference I have manually tested this on Android in an animated loan repayment calculator I'm building for a client. It involves animated props that are boolean, string (concatted and formatted currency values passed to TextInput native text prop, and react-native-svg animated path d prop with a few custom bezier curves), and a bunch of style props from width, color, fontSize, opacity and a few translate X and Y transforms. No issues with any of those so far. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#333>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACKXVS5V2DCISF4GQKGFALP6MDZ3ANCNFSM4H5VLW5A> .

[kmagiera]

After reiterating through this code a couple more times I think I grasped the idea and like the direction of simplifying props vs style node logic.

However with the presented approach I am not certain about few tiny aspects. For example how are we going to handle style node structure changes after rerenders? E.g. if you add new animated style prop when component rerenders. This is perhaps a rare usecase but for the sake of correctness we should support it.

Another idea that comes to my mind is that it may be possible to simplify this logic by getting rid of style nodes completely. In that approach we'd no longer need to special case them both in JS and native but only flatten styles into props directly on the JS side. What do you think?

[kmagiera]

Can we leave this change out of this PR. It does not seem relevant IMO

[simonbuerger]

Sure. For some context, the original point of the PR was to get string animated values working on Android. And without this change almost all the use cases for string animated Values actually break because the animated Double values display always with a decimal point even when they are whole integers (eg 12.0 rather than 12). Without this concat(new Value(12)) will return different results on iOS and Android - so should I rather put this particular change in another PR then?

[simonbuerger]

@kmagiera please confirm if I should still move this to a separate PR?

[simonbuerger]

@kmagiera I like the idea of flattening style and props before they even reach native. Let me have a go at it

[simonbuerger]

@kmagiera this is completely untested, but my first thought is that the only update required to do this is to update AnimatedProps.js (see an initial stab below)?

Aside from having to rip out all the traces of the separate style nodes code across native and JS of course (not a small task 😅)

import { findNodeHandle, StyleSheet } from 'react-native';

import AnimatedNode from './AnimatedNode';
import AnimatedEvent from './AnimatedEvent';
import { createOrReuseTransformNode } from './AnimatedTransform';

import invariant from 'fbjs/lib/invariant';
import deepEqual from 'fbjs/lib/areEqual';

function sanitizeProps(inputProps) {
  const props = {};
  for (const key in inputProps) {
    const value = inputProps[key];
    if (value instanceof AnimatedNode && !(value instanceof AnimatedEvent)) {
      props[key] = value.__nodeID;
    }
  }
  return props;
}

export function createOrReusePropsNode(props, callback, oldNode) {
  if (props.style) {
    let { style, ...rest } = props;
    style = StyleSheet.flatten(style) || {};
    if (style.transform) {
      style = {
        ...style,
        transform: createOrReuseTransformNode(
          style.transform,
          oldNode && oldNode._props.transform
        ),
      };
    }
    props = {
      ...rest,
      ...style,
    };
  }
  const config = sanitizeProps(props);
  if (oldNode && deepEqual(config, oldNode._config)) {
    return oldNode;
  }
  return new AnimatedProps(props, config, callback);
}

class AnimatedProps extends AnimatedNode {
  constructor(props, config, callback) {
    super(
      { type: 'props', props: config },
      Object.values(props).filter(n => !(n instanceof AnimatedEvent))
    );
    this._config = config;
    this._props = props;
    this._callback = callback;
    this.__attach();
  }

  _walkPropsAndGetAnimatedValues(props) {
    const updatedProps = {};
    for (const key in props) {
      const value = props[key];
      if (value instanceof AnimatedNode) {
        updatedProps[key] = value.__getValue();
      } else if (value && !Array.isArray(value) && typeof value === 'object') {
        // Support animating nested values (for example: shadowOffset.height)
        updatedProps[key] = this._walkPropsAndGetAnimatedValues(value);
      }
    }
    return updatedProps;
  }

  __onEvaluate() {
    return this._walkPropsAndGetAnimatedValues(this._props);
  }

  __detach() {
    const nativeViewTag = findNodeHandle(this._animatedView);
    invariant(
      nativeViewTag != null,
      'Unable to locate attached view in the native tree'
    );
    this._disconnectAnimatedView(nativeViewTag);
    super.__detach();
  }

  update() {
    this._callback();
  }

  setNativeView(animatedView) {
    if (this._animatedView === animatedView) {
      return;
    }
    this._animatedView = animatedView;

    const nativeViewTag = findNodeHandle(this._animatedView);
    invariant(
      nativeViewTag != null,
      'Unable to locate attached view in the native tree'
    );
    this._connectAnimatedView(nativeViewTag);
  }
}

[kmagiera]

Nice! Looking forward to more updates

[simonbuerger]

@kmagiera please review again. As suggested I have removed the StyleNode entirely and am now merging props and style props in AnimatedProps.js before they reach native

[jakub-gonet]

@simonbuerger, what's the status of this PR?

[jakub-gonet]

I'm closing this one due to inactivity and current Reanimated situation - we don't want to introduce big new changes to v1.