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

Use a closure to bind argument to callback in ReactErrorUtils #8363

Merged
merged 2 commits into from Nov 28, 2016

Conversation

Projects
None yet
4 participants
@aweary
Member

aweary commented Nov 20, 2016

See discussion starting here.

This prevents the fake event used here from being passed to the callback. This was causing issues where event handlers received an extra argument in dev only.

@gaearon I verified this fix resolves the issue by building and testing in the basic click counter example. I'm not sure where a unit test for this would go.

Use a closure to bind gaurded callback
This way the fake event isn't being implicitly passed into the event handler
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 20, 2016

Member

Feel free to add a test for ReactErrorUtils if one doesn't exist already. You could also look at change history to see if there were any related tests introduced with this file in the first place.

Can you confirm single argument matches production behavior?

Member

gaearon commented Nov 20, 2016

Feel free to add a test for ReactErrorUtils if one doesn't exist already. You could also look at change history to see if there were any related tests introduced with this file in the first place.

Can you confirm single argument matches production behavior?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Nov 28, 2016

Member

@gaearon I added test for ReactErrorUtils 👍

Member

aweary commented Nov 28, 2016

@gaearon I added test for ReactErrorUtils 👍

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 28, 2016

Member

Lint is failing.

Member

gaearon commented Nov 28, 2016

Lint is failing.

@gaearon gaearon added semver-minor and removed semver-patch labels Nov 28, 2016

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 28, 2016

Member

Tagging as minor since somebody might have accidentally relied on that.

Member

gaearon commented Nov 28, 2016

Tagging as minor since somebody might have accidentally relied on that.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 28, 2016

Member

Linter fails.

Member

gaearon commented Nov 28, 2016

Linter fails.

Add tests for ReactErrorUtils
Add fiber test report

Linting fixes
@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Nov 28, 2016

Member

@gaearon fixed 👍

Member

aweary commented Nov 28, 2016

@gaearon fixed 👍

@gaearon gaearon merged commit 6947db1 into facebook:master Nov 28, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.002%) to 83.676%
Details
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Nov 28, 2016

Member

LGTM

Member

gaearon commented Nov 28, 2016

LGTM

@gaearon gaearon modified the milestones: 15-hipri, 15-lopri Jan 6, 2017

@aweary aweary deleted the aweary:dev-event-handler-args branch Jan 26, 2017

acusti added a commit to brandcast/react that referenced this pull request Mar 15, 2017

Use a closure to bind argument to callback in ReactErrorUtils (facebo…
…ok#8363)

* Use a closure to bind gaurded callback

This way the fake event isn't being implicitly passed into the event handler

* Add tests for ReactErrorUtils

Add fiber test report

Linting fixes

@aweary aweary referenced this pull request Jul 11, 2017

Closed

React 15.6.2 Umbrella #10040

6 of 12 tasks complete
@Heedster

This comment has been minimized.

Show comment
Hide comment
@Heedster

Heedster Aug 8, 2017

@aweary @gaearon I am getting this issue in 15.6.1. Is this back ?
I see that ReactErrorUtils is now using the bind syntax which was replaced in this pull request.
https://github.com/facebook/react/blob/v15.6.1/src/renderers/shared/utils/ReactErrorUtils.js#L78

Is that causing the issue ?

Heedster commented Aug 8, 2017

@aweary @gaearon I am getting this issue in 15.6.1. Is this back ?
I see that ReactErrorUtils is now using the bind syntax which was replaced in this pull request.
https://github.com/facebook/react/blob/v15.6.1/src/renderers/shared/utils/ReactErrorUtils.js#L78

Is that causing the issue ?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Aug 8, 2017

Member

We didn’t cherry pick this change so it’s not in 15.x. We might release 15.6.2 with it, see #10040.

Member

gaearon commented Aug 8, 2017

We didn’t cherry pick this change so it’s not in 15.x. We might release 15.6.2 with it, see #10040.

@nhunzaker nhunzaker referenced this pull request Aug 10, 2017

Merged

15.6.2 Release Prep #10430

11 of 11 tasks complete

nhunzaker added a commit that referenced this pull request Aug 10, 2017

Use a closure to bind argument to callback in ReactErrorUtils (#8363)
* Use a closure to bind gaurded callback

This way the fake event isn't being implicitly passed into the event handler

* Add tests for ReactErrorUtils

Add fiber test report

Linting fixes

nhunzaker added a commit that referenced this pull request Aug 10, 2017

Use a closure to bind argument to callback in ReactErrorUtils (#8363)
* Use a closure to bind gaurded callback

This way the fake event isn't being implicitly passed into the event handler

* Add tests for ReactErrorUtils

Add fiber test report

Linting fixes

laurinenas added a commit to laurinenas/react that referenced this pull request May 28, 2018

Use a closure to bind argument to callback in ReactErrorUtils (facebo…
…ok#8363)

* Use a closure to bind gaurded callback

This way the fake event isn't being implicitly passed into the event handler

* Add tests for ReactErrorUtils

Add fiber test report

Linting fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment