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.

Sign up for GitHub

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

Jump to bottom

[Fiber] Fix to render falsy value as dangerouslySetInnerHTML #8652

Merged
merged 2 commits into from Jan 11, 2017
Merged

[Fiber] Fix to render falsy value as dangerouslySetInnerHTML #8652

merged 2 commits into from Jan 11, 2017

Conversation

koba04
Copy link
Contributor

@koba04 koba04 commented Dec 29, 2016 •
edited

This is to fix a bug which is unable to render falsy values(0 or '' or false) as dangerouslySetInnerHTML.

@koba04 koba04 force-pushed the fix-render-dangerouslySetInnerHTML branch from 150dd5e to 5493a36 Compare December 29, 2016 08:18
edvinerikson
edvinerikson reviewed Dec 29, 2016
View reviewed changes
src/renderers/dom/fiber/ReactDOMFiberComponent.js
@@ -398,10 +398,11 @@ function updateDOMProperties(
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
var nextHtml = nextProp ? nextProp[HTML] : undefined;
var lastHtml = lastProp ? lastProp[HTML] : undefined;
if (nextHtml) {
if (nextHtml || nextHtml === 0 || nextHtml === '') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe do a typeof instead? that is what we have done in other places where we need to detect numbers / strings. I am not sure this works with negative values

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! That makes sense. I'll update it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed it.

@koba04 koba04 force-pushed the fix-render-dangerouslySetInnerHTML branch from 5493a36 to f344590 Compare December 29, 2016 13:44
edvinerikson
edvinerikson reviewed Dec 29, 2016
View reviewed changes
src/renderers/dom/fiber/ReactDOMFiberComponent.js
@@ -398,10 +398,15 @@ function updateDOMProperties(
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
var nextHtml = nextProp ? nextProp[HTML] : undefined;
var lastHtml = lastProp ? lastProp[HTML] : undefined;
if (nextHtml) {
if (
nextHtml ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we even need this check now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for consistent with stack reconciler.
Because stack reconciler can render other than string and number.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay 👍

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about false? Does Stack render that?

@gaearon gaearon requested a review from acdlite December 30, 2016 01:24
acdlite
acdlite previously requested changes Jan 6, 2017
View reviewed changes
src/renderers/dom/fiber/ReactDOMFiberComponent.js
@@ -398,10 +398,15 @@ function updateDOMProperties(
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
var nextHtml = nextProp ? nextProp[HTML] : undefined;
var lastHtml = lastProp ? lastProp[HTML] : undefined;
if (nextHtml) {
if (
nextHtml ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about false? Does Stack render that?

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
@@ -44,6 +44,28 @@ describe('ReactDOMFiber', () => {
expect(container.textContent).toEqual('10');
});

it('should render dangerouslySetInnerHTML', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's put this test in ReactDOMComponent-test.js next to the other dangrouslySetInnerHTML tests. And give it a more specific name, like "can dangerouslySetInnerHTML with falsy value."

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for moving the test. Can you remove the ones in this file now? We don't need duplicates because ReactDOMComponent-test is run in both Stack and Fiber mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@acdlite OK, fixed it. I've added the tests using ReactDOM.render instead of _createContentMarkup into ReactDOMComponent-test.

@koba04 koba04 force-pushed the fix-render-dangerouslySetInnerHTML branch from f344590 to afc208f Compare January 6, 2017 04:35
@koba04
Copy link
Contributor Author

koba04 commented Jan 6, 2017

@acdlite Thank you for your review! I've put the tests for falsy values in ReactDOMComponent-test.js and fixed the behavior for false. In stack, dangrouslySetInnerHTML={{_html: false}} is rendered as false.

koba04
koba04 commented Jan 6, 2017
View reviewed changes
src/renderers/dom/fiber/ReactDOMFiberComponent.js
@@ -398,13 +398,10 @@ function updateDOMProperties(
} else if (propKey === DANGEROUSLY_SET_INNER_HTML) {
var nextHtml = nextProp ? nextProp[HTML] : undefined;
var lastHtml = lastProp ? lastProp[HTML] : undefined;
if (nextHtml) {
if (lastHtml) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this condition still need? It seems to be unnecessary because it is covered by if (lastHtml !== nextHtml).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm idk why that branch was there. Seems ok to remove it.

@koba04 koba04 force-pushed the fix-render-dangerouslySetInnerHTML branch from afc208f to 46d3e81 Compare January 10, 2017 01:22
@gaearon gaearon dismissed acdlite’s stale review January 11, 2017 21:16

concerns were addressed

@gaearon
Copy link
Collaborator

gaearon commented Jan 11, 2017

This looks good, thank you for fixing!

@gaearon gaearon merged commit beb5b74 into facebook:master Jan 11, 2017
@gaearon gaearon mentioned this pull request Jan 11, 2017
Merged
@koba04
Copy link
Contributor Author

koba04 commented Jan 12, 2017

Thank you!

@koba04 koba04 deleted the fix-render-dangerouslySetInnerHTML branch January 12, 2017 00:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@acdlite acdlite acdlite left review comments

@edvinerikson edvinerikson edvinerikson left review comments

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@koba04 @gaearon @acdlite @edvinerikson @facebook-github-bot