From 2a12730cc2a352852a8d786c95626a648fc4265a Mon Sep 17 00:00:00 2001 From: Mark Date: Mon, 29 Aug 2016 11:28:59 -0400 Subject: [PATCH 1/4] Make sure `props.children` is null for void elements like The error came from the fact that `props.children` would always be at minimum an empty array `[]`. This causes the following error: ```js ReactDOMServer.renderToString( Parser('') ); // Invariant Violation: img is a void element tag and must neither // have `children` nor use `dangerouslySetInnerHTML`. ``` The fix was to make `props.children` equal to `null` unless there were actually more DOM nodes inside the array. Add tests to confirm that void HTML element tags no longer throws an error. --- lib/dom-to-react.js | 4 ++-- test/data.json | 2 ++ test/dom-to-react.js | 17 +++++++++++++++++ 3 files changed, 21 insertions(+), 2 deletions(-) diff --git a/lib/dom-to-react.js b/lib/dom-to-react.js index 09d478e2..43135180 100644 --- a/lib/dom-to-react.js +++ b/lib/dom-to-react.js @@ -59,8 +59,8 @@ function domToReact(nodes, options) { if (node.name === 'textarea' && node.children[0]) { props.defaultValue = node.children[0].data; - } else if (node.children) { - // continue recursion of creating React elements + // continue recursion of creating React elements (if applicable) + } else if (node.children && node.children.length) { children = domToReact(node.children, options); } diff --git a/test/data.json b/test/data.json index e6f5b5a6..33be5b04 100644 --- a/test/data.json +++ b/test/data.json @@ -7,6 +7,8 @@ "complex": "Title
Header

Heading

Paragraph

Some text.
", "textarea": "", "script": "", + "img": "\"Image\"/", + "void": "
", "comment": "" }, "svg": { diff --git a/test/dom-to-react.js b/test/dom-to-react.js index 6722810c..4c0de6bf 100644 --- a/test/dom-to-react.js +++ b/test/dom-to-react.js @@ -5,6 +5,7 @@ */ var assert = require('assert'); var React = require('react'); +var ReactDOMServer = require('react-dom/server'); var htmlToDOMServer = require('../lib/html-to-dom-server'); var domToReact = require('../lib/dom-to-react'); var data = require('./data'); @@ -67,6 +68,22 @@ describe('dom-to-react parser', function() { ); }); + it('does not have `children` for void elements', function() { + var html = data.html.img; + var reactElement = domToReact(htmlToDOMServer(html)); + assert(!reactElement.props.children); + }); + + it('does not throw an error for void elements', function() { + var html = data.html.void; + var reactElements = domToReact(htmlToDOMServer(html)); + assert.doesNotThrow(function() { + ReactDOMServer.renderToStaticMarkup( + React.createElement('div', {}, reactElements) + ); + }); + }); + it('skips HTML comments', function() { var html = [data.html.single, data.html.comment, data.html.single].join(''); var reactElements = domToReact(htmlToDOMServer(html)); From db39cf6922645d0f30a2e2ba656880268b86218f Mon Sep 17 00:00:00 2001 From: Mark Date: Mon, 29 Aug 2016 12:10:01 -0400 Subject: [PATCH 2/4] Refactor tests by moving `render` function into helpers `render` is an alias for `ReactDOMServer.renderToStaticMarkup` Update `render` function to check that the first argument is a valid React element; otherwise, throw an error. --- test/dom-to-react.js | 6 ++---- test/helpers/index.js | 18 +++++++++++++++++- test/html-to-react.js | 26 ++++++++------------------ 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/test/dom-to-react.js b/test/dom-to-react.js index 4c0de6bf..1d68329b 100644 --- a/test/dom-to-react.js +++ b/test/dom-to-react.js @@ -5,9 +5,9 @@ */ var assert = require('assert'); var React = require('react'); -var ReactDOMServer = require('react-dom/server'); var htmlToDOMServer = require('../lib/html-to-dom-server'); var domToReact = require('../lib/dom-to-react'); +var helpers = require('./helpers/'); var data = require('./data'); /** @@ -78,9 +78,7 @@ describe('dom-to-react parser', function() { var html = data.html.void; var reactElements = domToReact(htmlToDOMServer(html)); assert.doesNotThrow(function() { - ReactDOMServer.renderToStaticMarkup( - React.createElement('div', {}, reactElements) - ); + helpers.render(React.createElement('div', {}, reactElements)); }); }); diff --git a/test/helpers/index.js b/test/helpers/index.js index e6cd4ba8..684ec69e 100644 --- a/test/helpers/index.js +++ b/test/helpers/index.js @@ -5,6 +5,21 @@ */ var assert = require('assert'); var util = require('util'); +var React = require('react'); +var ReactDOMServer = require('react-dom/server'); + +/** + * Render a React element to static HTML markup. + * + * @param {ReactElement} reactElement - The React element. + * @return {String} - The static HTML markup. + */ +function render(reactElement) { + if (!React.isValidElement(reactElement)) { + throw new Error(reactElement, 'is not a valid React element.'); + } + return ReactDOMServer.renderToStaticMarkup(reactElement); +} /** * Test for deep equality between objects that have circular references. @@ -65,5 +80,6 @@ function deepEqualCircular(actual, expected) { * Export assert helpers. */ module.exports = { - deepEqualCircular: deepEqualCircular + deepEqualCircular: deepEqualCircular, + render: render }; diff --git a/test/html-to-react.js b/test/html-to-react.js index 6ae575ef..4a84239c 100644 --- a/test/html-to-react.js +++ b/test/html-to-react.js @@ -5,20 +5,10 @@ */ var assert = require('assert'); var React = require('react'); -var ReactDOMServer = require('react-dom/server'); var Parser = require('../'); +var helpers = require('./helpers/'); var data = require('./data'); -/** - * Render a React element to static HTML markup. - * - * @param {ReactElement} reactElement - The React element. - * @return {String} - The static HTML markup. - */ -function render(reactElement) { - return ReactDOMServer.renderToStaticMarkup(reactElement); -} - /** * Tests for `htmlToReact`. */ @@ -44,21 +34,21 @@ describe('html-to-react', function() { it('converts single HTML element to React', function() { var html = data.html.single; var reactElement = Parser(html); - assert.equal(render(reactElement), html); + assert.equal(helpers.render(reactElement), html); }); it('converts single HTML element and ignores comment', function() { var html = data.html.single; // comment should be ignored var reactElement = Parser(html + data.html.comment); - assert.equal(render(reactElement), html); + assert.equal(helpers.render(reactElement), html); }); it('converts multiple HTML elements to React', function() { var html = data.html.multiple; var reactElements = Parser(html); assert.equal( - render(React.createElement('div', {}, reactElements)), + helpers.render(React.createElement('div', {}, reactElements)), '
' + html + '
' ); }); @@ -66,13 +56,13 @@ describe('html-to-react', function() { it('converts complex HTML to React', function() { var html = data.html.complex; var reactElement = Parser(html); - assert.equal(render(reactElement), html); + assert.equal(helpers.render(reactElement), html); }); it('converts SVG to React', function() { var svg = data.svg.complex; var reactElement = Parser(svg); - assert.equal(render(reactElement), svg); + assert.equal(helpers.render(reactElement), svg); }); }); @@ -94,7 +84,7 @@ describe('html-to-react', function() { } }); assert.equal( - render(reactElement), + helpers.render(reactElement), html.replace('Title', '') ); }); @@ -112,7 +102,7 @@ describe('html-to-react', function() { } }); assert.notEqual( - render(reactElement), + helpers.render(reactElement), html.replace( '', '

Heading

' From 226cc15285e9b1f68c57e5f269a4891a19bced1b Mon Sep 17 00:00:00 2001 From: Mark Date: Mon, 29 Aug 2016 12:41:18 -0400 Subject: [PATCH 3/4] Update `replace` method with additional `key` parameter The `replace` method now has 2 parameters: 1. domNode - The object describing the DOM node. 2. key - The array index. The `key` parameter was added because if the element had siblings, then the replaced React element should have a unique "key" prop or else React will display a warning: > Warning: Each child in an array or iterator should have a > unique "key" prop. See https://fb.me/react-warning-keys for > more information. Updated the mock data for complex html and used the `key` parameter to fix a test. --- lib/dom-to-react.js | 4 ++-- test/data.json | 2 +- test/html-to-react.js | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/dom-to-react.js b/lib/dom-to-react.js index 43135180..c0098d07 100644 --- a/lib/dom-to-react.js +++ b/lib/dom-to-react.js @@ -28,7 +28,7 @@ function domToReact(nodes, options) { // replace with custom React element (if applicable) if (isReplacePresent) { - replacement = options.replace(node); + replacement = options.replace(node, i); // i = key if (React.isValidElement(replacement)) { result.push(replacement); @@ -69,7 +69,7 @@ function domToReact(nodes, options) { continue; } - // specify a `key` prop if returning an array + // specify a "key" prop if element has siblings // https://fb.me/react-warning-keys if (len > 1) { props.key = i; diff --git a/test/data.json b/test/data.json index 33be5b04..48f259fe 100644 --- a/test/data.json +++ b/test/data.json @@ -4,7 +4,7 @@ "multiple": "

foo

bar

", "nested": "

foo bar

", "attributes": "
", - "complex": "Title
Header

Heading

Paragraph

Some text.
", + "complex": "Title
Header

Heading

Paragraph

Some text.
", "textarea": "", "script": "", "img": "\"Image\"/", diff --git a/test/html-to-react.js b/test/html-to-react.js index 4a84239c..4cffda1d 100644 --- a/test/html-to-react.js +++ b/test/html-to-react.js @@ -77,15 +77,15 @@ describe('html-to-react', function() { it('overrides the element if replace is valid', function() { var html = data.html.complex; var reactElement = Parser(html, { - replace: function(node) { + replace: function(node, key) { if (node.name === 'title') { - return React.createElement('meta', { charSet: 'utf-8' }); + return React.createElement('title', { key: key }, 'Replaced Title'); } } }); assert.equal( helpers.render(reactElement), - html.replace('Title', '') + html.replace('Title', 'Replaced Title') ); }); From e3c91acc3a0718a49d442181546fe0baff9645ed Mon Sep 17 00:00:00 2001 From: Mark Date: Mon, 29 Aug 2016 14:22:25 -0400 Subject: [PATCH 4/4] Update README with better documentation on `replace` method Add new parameter `key` and update examples. --- README.md | 49 +++++++++++++++++++++++++++++++++++-------------- 1 file changed, 35 insertions(+), 14 deletions(-) diff --git a/README.md b/README.md index ed05d200..4613a472 100644 --- a/README.md +++ b/README.md @@ -68,11 +68,36 @@ ReactDOM.render( ### Options -#### replace(domNode) +#### replace(domNode, key) -`replace` allows you to swap an element with your own React element. +The `replace` method allows you to swap an element with your own React element. -The `domNode` object has the same schema as the output from [htmlparser2.parseDOM](https://github.com/fb55/domhandler#example). +The method has 2 parameters: +1. `domNode`: An object which shares the same schema as the output from [htmlparser2.parseDOM](https://github.com/fb55/domhandler#example). +2. `key`: A number to keep track of the element. You should set it as the ["key" prop](https://fb.me/react-warning-keys) in case your element has siblings. + +```js +Parser('

text

', { + replace: function(domNode, key) { + console.log(domNode); + // { type: 'tag', + // name: 'p', + // attribs: { id: 'replace' }, + // children: [], + // next: null, + // prev: null, + // parent: null } + + console.log(key); // 0 + + return; + // element is not replaced because + // a valid React element is not returned + } +}); +``` + +Working example: ```js var Parser = require('html-react-parser'); @@ -81,18 +106,14 @@ var React = require('react'); var html = '

replace me

'; var reactElement = Parser(html, { - replace: function(domNode) { - // example `domNode`: - // { type: 'tag', - // name: 'p', - // attribs: { id: 'main' }, - // children: [], - // next: null, - // prev: null, - // parent: [Circular] } + replace: function(domNode, key) { if (domNode.attribs && domNode.attribs.id === 'main') { - // element is replaced only if a valid React element is returned - return React.createElement('span', { style: { fontSize: '42px' } }, 'replaced!'); + return React.createElement('span', { + key: key, + style: { fontSize: '42px' } }, + 'replaced!'); + // equivalent jsx syntax: + // return replaced!; } } });