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.

Sign up for GitHub

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

Jump to bottom

Remove .bind(this) from removePortal call #348

Merged
merged 1 commit into from
Mar 9, 2017
Merged

Remove .bind(this) from removePortal call #348

merged 1 commit into from
Mar 9, 2017

Conversation

mreishus
Copy link

@mreishus mreishus commented Mar 6, 2017

This fixes this warning from react in the console without breaking functionality:

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

Fixes #344.

This doesn't fix a bug or change the API, it should be basically a "silent" change.

Acceptance Checklist:

  • All commits have been squashed to one.
  • The commit message follows the guidelines in CONTRIBUTING.md.
  • Documentation (README.md) and examples have been updated as needed.
  • If this is a code change, a spec testing the functionality has been added.
  • If the commit message has [changed] or [removed], there is an upgrade path above.

@mreishus mreishus mentioned this pull request Mar 6, 2017
Closed
@diasbruno
Copy link
Collaborator

This test has failed:
keeps the modal in the DOM until closeTimeoutMS elapses.

Use npm run test.

diasbruno
diasbruno requested changes Mar 6, 2017
View reviewed changes
Copy link
Collaborator

@diasbruno diasbruno left a comment

Choose a reason for hiding this comment

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

Fix tests.

@mreishus
Copy link
Author

mreishus commented Mar 6, 2017

My fix was actually bad. It was causing removePortal to be run at the wrong time. I added a closure around it (amended commit and forced push to keep it all in one commit).

@mreishus
Copy link
Author

mreishus commented Mar 6, 2017

erm, I think I shouldn't have used es6.

@claydiffrient claydiffrient changed the base branch from master to v1 March 6, 2017 16:54
@claydiffrient
Copy link
Contributor

@mreishus I changed the base of this commit to the v1 branch since this problem only exists on v1.7.x.

If this fix was indeed intended to go to master and be part of v2 then please let me know and we can change that back.

@claydiffrient
Copy link
Contributor

Chances are you'll probably want to just rebase on v1 and do a forced push to clean things up though.

@mreishus
Copy link
Author

mreishus commented Mar 6, 2017

OK, redid off v1 branch, fixed tests and lint. I did use the () => es6 construction tho.

@diasbruno diasbruno added this to the 1.7 milestone Mar 6, 2017
@mreishus
Copy link
Author

mreishus commented Mar 7, 2017

Don't merge this please, it causes the same uglifyJS error that happened in issue 336.

@claydiffrient
Copy link
Contributor

I was thinking that it might @mreishus, my guess is it's the () =>

diasbruno
diasbruno requested changes Mar 8, 2017
View reviewed changes
Copy link
Collaborator

@diasbruno diasbruno left a comment

Choose a reason for hiding this comment

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

() => {} > function() {}.

Remove .bind(this) from removePortal call
114c724 
This fixes this warning from react in the console without breaking functionality:

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
@mreishus
Copy link
Author

mreishus commented Mar 8, 2017

done

@claydiffrient claydiffrient merged commit 185f2b0 into reactjs:v1 Mar 9, 2017
@claydiffrient
Copy link
Contributor

Released in 1.7.2

AndersDJohnson
AndersDJohnson reviewed Mar 9, 2017
View reviewed changes
lib/components/Modal.js
@@ -94,7 +94,7 @@ var Modal = React.createClass({
this.portal.closeWithTimeout();
}

setTimeout(this.removePortal.bind(this), closesAt - now);
setTimeout(function() { this.removePortal(); }, closesAt - now);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think removePortal won't be available on this in setTimeout which will have this context of window? If you don't use arrow function, it won't scope capture.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, you are right!

AndersDJohnson added a commit to AndersDJohnson/react-modal that referenced this pull request Mar 11, 2017
@AndersDJohnson
fix: remove portal context in timeout
30f9332 
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 pull request Mar 11, 2017
@AndersDJohnson
[fixed] remove portal context in timeout
017ab6b 
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 AndersDJohnson mentioned this pull request Mar 11, 2017
Merged
5 tasks
claydiffrient pushed a commit that referenced this pull request Mar 11, 2017
@AndersDJohnson @claydiffrient
[fixed] remove portal context in timeout (#353)
e1df119 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AndersDJohnson AndersDJohnson AndersDJohnson left review comments

@diasbruno diasbruno diasbruno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.7
Development

Successfully merging this pull request may close these issues.
4 participants
@mreishus @diasbruno @claydiffrient @AndersDJohnson