-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
15.6.2 Release Prep #10430
15.6.2 Release Prep #10430
Conversation
* 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
Is anything blocking #10025? I left a comment there: #10025 (comment). I think if that change is made (and code only touches Stack rather than Fiber) then the change is good and we should get it in. |
I didn't think to comment in this thread. I have reverted the Fiber changes and updated my tests. Everything is passing. I've been trying not to bug Dan because I think he's on vacation. 😀 |
Awesome. @iansu thanks! |
__DEV__ = originalDev; | ||
process.env.NODE_ENV = originalEnv; | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Controlling for process.env.NODE_ENV
is new here. Otherwise, I was unable to get the invariant to minify the warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way it is written here will break other things in non-trivial way. Please check ReactDOMProduction for example of test that overrides process.env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact it would be great if you could send a PR to master that fixes DEV override to work same way as in ReactDOMProduction-test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've done that in 26722ce and added a TODO to follow up with a similar change on master.
Everything checked off. I believe this is good to go. |
CHANGELOG.md
Outdated
* Prevent event handlers from receiving extra argument (dev only) ([@aweary](https://github.com/aweary) in [#10115](https://github.com/facebook/react/pull/8363) | ||
* Fix cases where onChange would not fire with defaultChecked on radio inputs ([@jquense](https://github.com/jquense) in [#10156](https://github.com/facebook/react/pull/10156)) | ||
* Add support for controlList attribute to DOM property whitelist ([@nhunzaker](https://github.com/nhunzaker) in [#9940](https://github.com/facebook/react/pull/9940)) | ||
* Fix a bug where creating an element with a ref in a constructor threw an error in dev mode ([@iansu](https://github.com/iansu) in [#10025](https://github.com/facebook/react/pull/10025)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is wrong. The bug was that creating an element with a ref in a constructor did NOT throw an error in dev mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙈 Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it in da62f42
Any update when this will be available? I'm looking to get the fixes for uncontrolled radio buttons. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, pending the process.env.NODE_ENV
stuff @nhunzaker and @gaearon were discussing.
Any update when this will be available? |
Hi @gaearon / @aweary / @nhunzaker, I know you guys aren't actively working on this branch as you're working on on the next version but I was wondering if it would be possible to get this wrapped up and merged? There are several folks waiting on this to fix issues in their app (including us). If there is any more action that needs to be taken, I'd be more than happy take the lead to get this done. Thanks! |
@colebowl Thanks for the prod, and I am sorry it was necessary. I've updated the ref test to use the recommended code and I've updated the timestamps on the changelog and blog post. This should be good to go, but I'll be out of pocket the rest of the day. I'm happy to review any updates if we need to change anything. I'll circle back later tonight. |
We'll need to get the LICENSE change rolled here. Plan to get it out tomorrow. |
I can just do it after merging this. Probably easiest that way. |
… Only set ReactCurrentOwner.current in dev mode when the component has no constructor. (#10025) nhunzaker: I've added an additional line to the ref test that sets the NODE_ENV invironment variable. This allows the test to pass.
f69d2ef
to
5634f0d
Compare
Commits
Documentation
Misc