-
Notifications
You must be signed in to change notification settings - Fork 77
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
Protect void elements from translations which try to set their children #155
Conversation
828b1b0
to
6ce6ca3
Compare
@hkasemir Would you mind reviewing this? |
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.
I had a thought about if we should consider reporting the improperly formatted message, but otherwise, looks good to me!
// "void element tags must neither have `children` nor use | ||
// `dangerouslySetInnerHTML`" error. | ||
if (elem.type in VOID_ELEMENTS) { | ||
return cloneElement(elem, localizedProps); |
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.
any chance we'd want to log a warning to the console to indicate an invalid message format? Or do we prefer silently handling the error?
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.
I'd like to tackle the problem of error reporting in a separate PR.
); | ||
|
||
assert.ok(wrapper.contains( | ||
<input title="TITLE" /> |
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.
👍
'source': true, | ||
'track': true, | ||
'wbr': true | ||
}; |
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.
also a quick search of void elements produced a larger list than this:
https://github.com/wooorm/html-void-elements/
any reason we're using this smaller list?
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.
though this does look suspiciously exactly like what is in the React code: https://github.com/facebook/react/blob/master/packages/react-dom/src/shared/omittedCloseTags.js
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.
Ah, nice find in React's source. I'll use their file instead of the void-elements
package because what we really care about here is not a complete list of void elements but rather the list that React uses to throw errors.
08d6fbc
to
2874a99
Compare
A broken translation may accidentally set a value which <Localized> will normally insert as a child of its wrapped component. For void elements we need to actively forbid this because React throws when trying to set children of void elements, breaking the entire app.
2874a99
to
67cd61f
Compare
A broken translation may accidentally set a value which
<Localized>
will normally insert as a child of its wrapped component. For void elements we need to actively forbid this because React throws when trying to set children of void elements, breaking the entire app.