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

Issue: 5013 Added necessary code for firing warning if value is null #5048

Merged
merged 1 commit into from Oct 15, 2015

Conversation

Projects
None yet
6 participants
@antsmartian
Copy link
Contributor

commented Oct 4, 2015

Fix of the issue #5013. Let me know if anything else needs to be done. Thanks.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 4, 2015

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 4, 2015

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@@ -150,6 +161,8 @@ var ReactDOMSelect = {
checkSelectPropTypes(inst, props);
}

warnIfValueIsNull(props);

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 5, 2015

Contributor

This should go into the if(__DEV__) check above. We only fire warnings in dev mode.

function warnIfValueIsNull(props) {
var valueLink = (props != null && props.valueLink != undefined) ? props.valueLink.value : '';

if (props != null && props.value === null || valueLink === null) {

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 5, 2015

Contributor

We probably don't need to be checking valueLink, since that is going to be deprecated in 0.15 anyway.

@@ -44,6 +44,17 @@ function getDeclarationErrorAddendum(owner) {
return '';
}

//https://github.com/facebook/react/issues/5013

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 5, 2015

Contributor

We don't generally put links to the github issues in the source. It clutters up the code, and it's easy enough to find the issue in github.

var link = new ReactLink(null, mocks.getMockFunction());
ReactTestUtils.renderIntoDocument(<select valueLink={link}><option value="test"/></select>);
expect(console.error.argsForCall.length).toBe(1);

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 5, 2015

Contributor

We should assert that the console.error contains the expected error message. See how I did this in https://github.com/facebook/react/pull/5032/files

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2015

We should make sure we only warn once for each error message. Otherwise, for an app that renders often, we will fill the error logs with 10,000 copies of this message. See how I did this in https://github.com/facebook/react/pull/5032/files with variables like didWarnCheckedLink

@jimfb jimfb added this to the 0.15 milestone Oct 5, 2015

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2015

@antoaravinth Ok, we're merging 0.15 stuff now. Can you rebase and fix the feedbacks above? Thanks!

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2015

@jimfb : Sure. Once the merging is done, I can do the rebase and fix the issues that you have mentioned as well. Thanks.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2015

@antoaravinth Most of the 0.15 stuff is merged. I don't see any merge conflicts yet (I'm actually a little surprised, I expected #5032 to conflict).

Anyway, we should go ahead and update this PR with the feedback above before merging this change in. You can do that by pushing your changes to the same branch in your github fork.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2015

@jimfb: Sure will do the same.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

@jimfb : Thanks for your help, I have rebased and pushed the necessary changes. Kindly let me know if anything else is pending from my side.

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 14, 2015

@antoaravinth updated the pull request.

if (props != null && props.value === null && !didWarnValueNull) {
warning(false, '`value` prop on `textarea` should not be null.' +
' Consider using the empty string to clear the component' +
' `undefined` for uncontrolled components.');

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 14, 2015

Contributor

There is a missing " or " between component and undefined

ReactTestUtils.renderIntoDocument(<input type="text" value={null} />);
expect(console.error.argsForCall[0][0]).toContain(
'`value` prop on `input` should not be null. Consider using the empty string to clear the component'
+ ' `undefined` for uncontrolled components.'

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 14, 2015

Contributor

" or "

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

@pluma Looks like there are still two commits in this PR. Maybe try squashing one more time?

@spicyj This looks good to me. Any last change requests before we merge?

@pluma

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

@jimfb Yeah, that's because you're looking at the wrong PR 😉

Hint: #5140 is the one you should be looking at.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2015

Oops, correct PR, wrong person. I meant to ping @antoaravinth

function warnIfValueIsNull(props) {

if (props != null && props.value === null && !didWarnValueNull) {
warning(false, '`value` prop on `input` should not be null.' +

This comment has been minimized.

Copy link
@zpao

zpao Oct 14, 2015

Member

This formatting doesn't match our style, see any other warning or invariant callsite for examples.

Also, please remove the empty line above the condition.

Edit: further details on formatting long strings like this: blank space should go at the end of the preceding line.

'foo bar. ' +
'baz'

ReactTestUtils.renderIntoDocument(<textarea value={null} />);
expect(console.error.argsForCall[0][0]).toContain(
'`value` prop on `textarea` should not be null. Consider using the empty string to clear the component'

This comment has been minimized.

Copy link
@zpao

zpao Oct 14, 2015

Member

Is this indented 3 spaces? How is lint not catching that…

Issue: 5013 Added necessary code for firing warning if value is null
Fixed the lint issues

Added logic for handling the warning only once and added the test cases for the same. Also moved the warning part to only DEV mode

Changed few lines related to the formatting issues

Removing the empty whitespace
@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 15, 2015

@antoaravinth updated the pull request.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

commented Oct 15, 2015

@zpao @jimfb Done, let me know if anything else is needed from my side.

jimfb added a commit that referenced this pull request Oct 15, 2015

Merge pull request #5048 from antoaravinth/PR-5013
Issue: 5013 Added necessary code for firing warning if value is null

@jimfb jimfb merged commit b735dd4 into facebook:master Oct 15, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2015

Looks great, thanks @antoaravinth!

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Oct 15, 2015

This warning should really include the owner.

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2015

@spicyj Yeah, that would be good. How would we feel about including the parent/debug path from #5167?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.