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

PropTypes: distinguish nullable from optional object field #7291

Merged
merged 3 commits into from Jul 26, 2016

Conversation

Projects
None yet
5 participants
@chenglou
Contributor

chenglou commented Jul 16, 2016

This gives a more precise message (no type semantics change) to the case of passing a field in an object, but whose value is null:

Before:

propTypes: {
  foo: React.PropTypes.number.isRequired
}

Would scream "Required prop foo was not specified in MyComp".

Now it'll be "Required prop foo was specified in MyComp, but its value is null.".

Works as expected in nested objects.

This fixes the issue of a component transitively passing a null, specifying the correct field to the child but have the child tell it that it didn't provide the prop.

Optional field and nullable are two different things anyway.

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Jul 16, 2016

Contributor

cc @spicyj @keyanzhang realized this confusion during the codemod.
cc @zpao look at me I'm contributing to React!

Contributor

chenglou commented Jul 16, 2016

cc @spicyj @keyanzhang realized this confusion during the codemod.
cc @zpao look at me I'm contributing to React!

@ghost ghost added the CLA Signed label Jul 16, 2016

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang

keyanzhang Jul 16, 2016

Member

I like this! Related: #3163 and #2166

Member

keyanzhang commented Jul 16, 2016

I like this! Related: #3163 and #2166

null,
ReactPropTypesSecret
);
expect(error2 instanceof Error).toBe(true);

This comment has been minimized.

@gaearon

gaearon Jul 16, 2016

Member

Nit but I guess you’d want to do the instanceof check for error1 too?

@gaearon

gaearon Jul 16, 2016

Member

Nit but I guess you’d want to do the instanceof check for error1 too?

This comment has been minimized.

@chenglou

chenglou Jul 17, 2016

Contributor

Nice catch, corrected, thanks.

@chenglou

chenglou Jul 17, 2016

Contributor

Nice catch, corrected, thanks.

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jul 17, 2016

Member

To be clear, this distinguishes null from undefined, but does not distinguish undefined from an absent field – right?

Member

sophiebits commented Jul 17, 2016

To be clear, this distinguishes null from undefined, but does not distinguish undefined from an absent field – right?

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Jul 17, 2016

Contributor

Right, I don't tackle that.

Contributor

chenglou commented Jul 17, 2016

Right, I don't tackle that.

@chenglou

This comment has been minimized.

Show comment
Hide comment
@chenglou

chenglou Jul 26, 2016

Contributor

@spicyj is this good for merging?

Contributor

chenglou commented Jul 26, 2016

@spicyj is this good for merging?

Show outdated Hide outdated src/isomorphic/classic/types/ReactPropTypes.js
@@ -144,6 +144,12 @@ function createChainableTypeChecker(validate) {
if (props[propName] == null) {
var locationName = ReactPropTypeLocationNames[location];
if (isRequired) {
if (props[propName] === null) {
return new Error(
`Required ${locationName} \`${propFullName}\` was specified in ` +

This comment has been minimized.

@sophiebits

sophiebits Jul 26, 2016

Member

Can we do

The ${locationName} `${propFullName}` is required in ${componentName}, but its value is null.

and replace the other message with "…its value is undefined."?

lgtm. Can you send me a diff internally to html/shared/core/createWarning.js adding the new wording so we don't forget?

@sophiebits

sophiebits Jul 26, 2016

Member

Can we do

The ${locationName} `${propFullName}` is required in ${componentName}, but its value is null.

and replace the other message with "…its value is undefined."?

lgtm. Can you send me a diff internally to html/shared/core/createWarning.js adding the new wording so we don't forget?

@sophiebits

This comment has been minimized.

Show comment
Hide comment
@sophiebits

sophiebits Jul 26, 2016

Member

I'd like the wording to be clearer and more accurate as noted inline, but after you fix that feel free to squash and merge.

Member

sophiebits commented Jul 26, 2016

I'd like the wording to be clearer and more accurate as noted inline, but after you fix that feel free to squash and merge.

@ghost ghost added the CLA Signed label Jul 26, 2016

@sophiebits sophiebits added this to the 15-next milestone Jul 26, 2016

chenglou added some commits Jul 16, 2016

PropTypes: distinguish nullable from optional object field
This gives a more precise message (no type semantics change) to the case of passing a field in an object, but whose value is `null`:

Before:

```js
propTypes: {
  foo: React.PropTypes.number.isRequired
}
```

Would scream "Required prop `foo` was not specified in `MyComp`".

Now it'll be "Required prop `foo` was specified in `MyComp`, but its value is `null`.".

Works as expected in nested objects.

This fixes the issue of a component transitively passing a `null`, specifying the correct field to the child but have the child tell it that it didn't provide the prop.

Optional field and nullable are two different things anyway.

@ghost ghost added the CLA Signed label Jul 26, 2016

@ghost ghost added the CLA Signed label Jul 26, 2016

@chenglou chenglou merged commit 0292d34 into facebook:master Jul 26, 2016

1 check passed

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

@chenglou chenglou deleted the chenglou:pt branch Jul 26, 2016

@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016

zpao added a commit that referenced this pull request Oct 4, 2016

PropTypes: distinguish nullable from optional object field (#7291)
* PropTypes: distinguish nullable from optional object field

This gives a more precise message (no type semantics change) to the case of passing a field in an object, but whose value is `null`:

Before:

```js
propTypes: {
  foo: React.PropTypes.number.isRequired
}
```

Would scream "Required prop `foo` was not specified in `MyComp`".

Now it'll be "Required prop `foo` was specified in `MyComp`, but its value is `null`.".

Works as expected in nested objects.

This fixes the issue of a component transitively passing a `null`, specifying the correct field to the child but have the child tell it that it didn't provide the prop.

Optional field and nullable are two different things anyway.

* Add missing test case.

* Reword messages.

(cherry picked from commit 0292d34)

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v16 #29

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