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

getting bind warning from react after 1.7.0 #344

Closed
mreishus opened this issue Mar 2, 2017 · 10 comments
Closed

getting bind warning from react after 1.7.0 #344

mreishus opened this issue Mar 2, 2017 · 10 comments

Comments

@mreishus
Copy link

mreishus commented Mar 2, 2017

Summary:

I'm getting this warning from react in the console:

Warning: bind(): You are binding a component method to the component. React does this for you automatically in a high-performance way, so you can safely remove this call. See Modal

Here is the call where's it happening (look for the arrow in comments on right side)

  componentWillUnmount: function() {
    if (this.props.ariaHideApp) {
      ariaAppHider.show(this.props.appElement);
    }

    var state = this.portal.state;
    var now = Date.now();
    var closesAt = state.isOpen && this.props.closeTimeoutMS
      && (state.closesAt
        || now + this.props.closeTimeoutMS);

    if (closesAt) {
      if (!state.beforeClose) {
        this.portal.closeWithTimeout();
      }

      setTimeout(this.removePortal.bind(this), closesAt - now);   /// <-- This line
    } else {
      this.removePortal();
    }
  },

Steps to reproduce:

I think it happens when I unmount a modal when it's still disappearing.

@feux07
Copy link

feux07 commented Mar 5, 2017

I also got this warning when use with more than one modal on same page.

@diasbruno
Copy link
Collaborator

diasbruno commented Mar 5, 2017

This can be due to some double bind(). Can you guys make a PR for this?
If changing this.removePortal.bind(this) to this.removePortal works, than it's a fix.

@mreishus
Copy link
Author

mreishus commented Mar 6, 2017

Tested, works fine, made pull request #348

@mreishus mreishus closed this as completed Mar 6, 2017
@diasbruno
Copy link
Collaborator

diasbruno commented Mar 9, 2017

[edit] actually, it need to be confirmed.
#348 is not a fix, unfortunately. @adjohnson916 spotted the 'bug' when referencing this inside of the callback of the setTimeout's function. see he's comment on the PR.

@diasbruno diasbruno reopened this Mar 9, 2017
@mreishus
Copy link
Author

mreishus commented Mar 9, 2017

yeah, I don't know how to fix then.

bind -> react gives warning
don't bind -> runs at wrong time, test fails
use es6 -> works w/o warning but uglify fails

@diasbruno
Copy link
Collaborator

diasbruno commented Mar 9, 2017

yeah, this is kinda sad. this mixture of es5/6.

  componentWillUnmount: function() {
    var modal = this;
    {...}
    // yes, old style closure...
    setTimeout(function() { console.log(this, modal); modal.removePortal(); }, 1);
  }

@claydiffrient
Copy link
Contributor

I think that just having bind there is probably the best for now. The warning shouldn't cause problems. I mean the goal is that 1.7 is a release to appease people as an intermediate to 2.0. Backporting from a ES2015+ codebase to a ES5 codebase is messy.

AndersDJohnson added a commit to AndersDJohnson/react-modal that referenced this issue Mar 11, 2017
Fixes the binding of the `removePortal` call in the `setTimeout` callback
by capturing a this reference in a variable in the closure scope.
Doesn't use `bind` which apparently causes warnings.

Fixes reactjs#344, fixes reactjs#348
AndersDJohnson added a commit to AndersDJohnson/react-modal that referenced this issue Mar 11, 2017
Fixes the binding of the `removePortal` call in the `setTimeout` callback
by capturing a this reference in a variable in the closure scope.
Doesn't use `bind` which apparently causes warnings.

Fixes reactjs#344, fixes reactjs#348
@AndersDJohnson
Copy link
Contributor

@claydiffrient @diasbruno @mreishus I just submitted PR #353 which should solve this.

claydiffrient pushed a commit that referenced this issue Mar 11, 2017
Fixes the binding of the `removePortal` call in the `setTimeout` callback
by capturing a this reference in a variable in the closure scope.
Doesn't use `bind` which apparently causes warnings.

Fixes #344, fixes #348
@diasbruno
Copy link
Collaborator

diasbruno commented Mar 22, 2017

Since this issue has a fix, I'm closing this issue.

Thanks @mreishus and @adjohnson916 for taking care of this one.

@mreishus
Copy link
Author

thanks, sorry about the mess, I was just trying to make things easier~

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

No branches or pull requests

5 participants