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

Should not coerce children prop on custom elements to a string. Fixes #5088 #5093

Merged
merged 1 commit into from Oct 30, 2015

Conversation

Projects
None yet
5 participants
@jimfb
Copy link
Contributor

commented Oct 8, 2015

Should not coerce children prop (or any prop, really) on custom elements to a string. Fixes #5088

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch from a703c37 to ef85ba2 Oct 8, 2015

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch from ef85ba2 to 530b7f7 Oct 8, 2015

@jimfb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2015

@sophiebits

This comment has been minimized.

Copy link
Collaborator

commented Oct 8, 2015

Should we just treat children (and dangerouslySetInnerHTML) specially? It seems maybe-useful to cast other things to string.

@jimfb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2015

Yeah, that's what my original diff (jimfb@a703c37) did, but then I thought about it and most of the coercions were mistakes. There are situations where we might want to treat non-string values differently (for instance, a function property following the pattern onXXX might be treated as an event handler). Figured we could always relax the coercion requirement in the future, but for now requiring it to be a string/number seems like a reasonable restriction. If you still want it changed, let me know, I don't have a strong preference.

@jimfb jimfb modified the milestones: 0.14.1, 0.15 Oct 9, 2015

@jimfb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 10, 2015

Ping @spicyj. Have I convinced you that we should require all webcomponent props to be strings/numbers, and perhaps back off on this requirement later. Or would you prefer to go with the older diff (jimfb@a703c37). Or what's your thinking?

@sebmarkbage

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

Regardless, this is a breaking change so it should not be a patch level release. It can go into 0.15.

@jimfb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2015

@sebmarkbage Addresses a regression that we introduced; namely that we now set the children attribute to [Object object] for all web components.

@jimfb jimfb closed this Oct 12, 2015

@jimfb jimfb reopened this Oct 12, 2015

@sebmarkbage

This comment has been minimized.

Copy link
Member

commented Oct 12, 2015

Then we should do what @spicyj suggested and your original.

JS has lots of coercion. It's just how the language works. Silently ignoring isn't necessarily better. It is probably worse here since it becomes a breaking change rather than an isolated patch of the regression.

If you want to protect against it, use Flow or something like Restrict Mode on top.

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch 2 times, most recently from 27cf905 to 0ae1fba Oct 12, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 12, 2015

@jimfb updated the pull request.

@jimfb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2015

@spicyj @sebmarkbage PR updated; ready to merge?

if (this._tag != null && isCustomComponent(this._tag, props)) {
markup = DOMPropertyOperations.createMarkupForCustomAttribute(propKey, propValue);
if (propKey != 'children' || typeof propValue === 'string' || typeof propValue === 'number') {

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Oct 17, 2015

Member

Plain string comparison is not closure compiler compatible.

We should ignore it even if it's a string/number.

Otherwise this won't work:

<custom-element>My Text Content</custom-element>

Also, could you add some tests that actually involve children like the issue that you're fixing.

This comment has been minimized.

Copy link
@zpao

zpao Oct 27, 2015

Member

To get past the closure compiler issue you can use var CHILDREN = keyOf({children: null}) and then use that in place of the string literal.

That's the safest thing, though children might actually be in the DOM externs.

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch from 0ae1fba to f12fd6b Oct 27, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 27, 2015

@jimfb updated the pull request.

if (this._tag != null && isCustomComponent(this._tag, props)) {
markup = DOMPropertyOperations.createMarkupForCustomAttribute(propKey, propValue);
if (propKey != CHILDREN || typeof propValue === 'string' || typeof propValue === 'number') {

This comment has been minimized.

Copy link
@zpao

zpao Oct 27, 2015

Member

!== (looks like we might have relaxed that when switching to fbjs lint rules)

@spicyj's long line length detector is slipping…

@zpao

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

@jimfb I'm going to do another 0.14.2 soon so if you can fix up the last few things we can probably get this in there.

Although… I don't see history of when this got changed from 0.15 to 0.14.1, is it actually safe?

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch from f12fd6b to 42211a9 Oct 30, 2015

@jimfb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2015

Done. I rebased. This was a regression introduced in 0.14, so it should be safe.

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch from 42211a9 to 3769cb3 Oct 30, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 30, 2015

@jimfb updated the pull request.

@zpao

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

Well, this doesn't quite bring us back to the same behavior of 0.13. So regression… sort of yes. But the changes here don't "fix" everything. I don't know what's right.

I think there's also an argument to be made for explicit use of the children prop vs the rest arg use. The rest arg use has clear intention of putting them as actual child nodes. The children prop is less clear (and unfortunately a big cloudy when we consider composition and {...this.props}).

React.renderToStaticMarkup(<my-component>5</my-component>)
React.renderToStaticMarkup(<my-component children={5}></my-component>)
React.renderToStaticMarkup(<my-component>{[5]}</my-component>)

// 0.13.3
'<my-component>5</my-component>'
'<my-component>5</my-component>'
'<my-component>5</my-component>'

0.14.1
'<my-component children="5">5</my-component>'
'<my-component children="5">5</my-component>'
'<my-component children="5">5</my-component>'

// With this
'<my-component children="5">5</my-component>'
'<my-component children="5">5</my-component>'
'<my-component>5</my-component>'

As you can see, you've only changed that last one. Which is consistent with the "not coercing children" issue as filed. Maybe that's correct and I should shut up, it just seems weird.

@jimfb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2015

@zpao So the "correct" behavior, imo, is to have <my-component children="5" /> != <my-component>5</my-component> at the element level. It's fine if you want to merge children into props.children before passing them into composite components in order to preserve API, but at the element level they should be separate.

Sebastian wants (at least for now) us to drop the typeof checks, so I'll do that now.

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch from 3769cb3 to 0595263 Oct 30, 2015

@@ -742,9 +743,11 @@ ReactDOMComponent.Mixin = {
}
propValue = CSSPropertyOperations.createMarkupForStyles(propValue, this);
}
var markup = null;
var markup = '';

This comment has been minimized.

Copy link
@sebmarkbage

sebmarkbage Oct 30, 2015

Member

Why is this change needed?

This comment has been minimized.

Copy link
@jimfb

jimfb Oct 30, 2015

Author Contributor

No idea. Presumably I wouldn't have made it if it wasn't necessary, but I don't see the reason for it and it passes unit tests as null so I'll go ahead and remove it.

@jimfb jimfb force-pushed the jimfb:avoid-children-to-string-coercion branch from 0595263 to 86fc947 Oct 30, 2015

@facebook-github-bot

This comment has been minimized.

Copy link

commented Oct 30, 2015

@jimfb updated the pull request.

@sebmarkbage

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

I think that it is likely that if we wanted to "standardize" the outline of a React Element, that other people might have opinion on this part of the structure. If it becomes necessary, I'd like to punt changing the React Element outline until that point.

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

Merge pull request #5093 from jimfb/avoid-children-to-string-coercion
Should not coerce children prop on custom elements to a string.  Fixes #5088

@jimfb jimfb merged commit a2d26c8 into facebook:master Oct 30, 2015

1 check passed

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

@zpao zpao modified the milestones: 0.14.2, 0.14.x Nov 2, 2015

zpao added a commit to zpao/react that referenced this pull request Nov 2, 2015

Merge pull request facebook#5093 from jimfb/avoid-children-to-string-…
…coercion

Should not coerce children prop on custom elements to a string.  Fixes facebook#5088
(cherry picked from commit a2d26c8)

alunyov pushed a commit to alunyov/react that referenced this pull request Mar 10, 2016

Don't interpret reserved props as attributes for SVG elements
This fixes a bug introduced in facebook#5714.
The code paths are now similar to web components where we worked around the same issue in facebook#5093.

Additionally, we also skip dangerouslySetInnerHTML and suppressContentEditableWarning.

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

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
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.