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

[LayoutManager] MessageQueueMixin._invokeCallback : do not call cb when it does not exist #984

Closed
wants to merge 1 commit into from

Conversation

machard
Copy link
Contributor

@machard machard commented Apr 23, 2015

I'm not sure if it is intentional to call cb even when it does not exist.

As it is calling warning just before, i would say it should just stay a warning and should not throw.

I will try to find why i get this warning, but until then this fix allows me to run my app correctly

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 23, 2015
@tadeuzagallo
Copy link
Contributor

Hey, it's really supposed to be there... if it's being triggered there's probably something really bad happening. Which version are you running? I have a commit that hasn't been synced yet that should make it almost impossible to happen...

Do you ever call the same RCTResponseSenderBlock twice?

@machard
Copy link
Contributor Author

machard commented Apr 25, 2015

I'm running last version 0.4.0

I've managed to have a mini example illustrating the problem.
The presence of a wrapping View seems to be causing it.

My use case is to have a scaling animation of the word when entering the view

var Word = React.createClass({
  componentWillUpdate (nextProps, nextState) {
      LayoutAnimation.configureNext({
        duration: 300,
        create: {
          type: LayoutAnimation.Types.easeInEaseOut,
          property: LayoutAnimation.Properties.scaleXY,
        }
      }, () => {}, () => {});
  },

  componentDidMount: function() {
    this.setState({
      word : "word"
    });
  },

  render: function() {
    return (
      <Text>
        {this.state && this.state.word}
      </Text>
    );
  }
});

var Container = React.createClass({
  render: function() {
    return (
      <View>
        <Word />
      </View>
    );
  }
});

/*
AppRegistry.registerComponent('callbackBug', () => Container); // will not work
AppRegistry.registerComponent('callbackBug', () => Word); // will work
*/

@tadeuzagallo
Copy link
Contributor

This is an issue with the LayoutManager actually. /cc @a2 Apparently the - [RCTUIManager uiBlockWithLayoutUpdateForRootView:] calls the onComplete callback once per new view, when you added a second view, it was called twice, hence the error.

On the other hand, this error should not be possible after 0e67e33, so on the next update it shouldn't affect users anymore.

@sahrens
Copy link
Contributor

sahrens commented Apr 25, 2015

LayoutAnimation has some rough edges and needs some polish - it works as a global config for the next layout transaction, which will typically affect multiple views, so the callback should only be called once per transaction (that's just a bug), but also if you try to configure multiple conflicting LayoutAnimations for the same transaction, the callbacks might also get messed up and it's not completely clear what the right behavior is in that case.

cc @nicklockwood as well.

On Apr 25, 2015, at 7:27 AM, Tadeu Zagallo notifications@github.com wrote:

This is an issue with the LayoutManager actually. /cc @a2 Apparently the - [RCTUIManager uiBlockWithLayoutUpdateForRootView:] calls the onComplete callback once per new view, when you added a second view, it was called twice, hence the error.

On the other hand, this error should not be possible after 0e67e33, so on the next update it shouldn't affect users anymore.


Reply to this email directly or view it on GitHub.

@nicklockwood
Copy link
Contributor

LayoutAnimation is a neat feature, but until we work out what our overall animation strategy is, I don't really want to invest a lot of time in it. It feels like an iOS-specific hack, and I'm not sure if we'll be able to support it x-platform in its current form.

@sahrens
Copy link
Contributor

sahrens commented Apr 25, 2015

No reason we can't implement it on other platforms, even web. Fundamentally all it's doing is animating the frames computed by the layout algorithm. We may want to change the way the animation curves are defined, but that's just a detail.

On Apr 25, 2015, at 12:41 PM, Nick Lockwood notifications@github.com wrote:

LayoutAnimation is a neat feature, but until we work out what our overall animation strategy is, I don't really want to invest a lot of time in it. It feels like an iOS-specific hack, and I'm not sure if we'll be able to support it x-platform in its current form.


Reply to this email directly or view it on GitHub.

@nicklockwood
Copy link
Contributor

Right, the spring animation curve in particular seems very platform-specific, but I agree the general principle could be more widely applicable. And it's declarative, which fits nicely with React.

@brentvatne brentvatne changed the title MessageQueueMixin._invokeCallback : do not call cb when it does not exist [LayoutManager] MessageQueueMixin._invokeCallback : do not call cb when it does not exist Jun 1, 2015
@brentvatne
Copy link
Collaborator

Closing because 0e67e33 landed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants