Improve error behavior of Danger.dangerouslyRenderMarkup.
HTML comment nodes are now interspersed in the original markup list so that we can tell which chunks of markup rendered as more or less than one node, and give a more helpful error message in that case. Regardless of how many nodes were rendered, we only take the first one. If zero nodes were rendered, then `resultList` will contain `null` at the corresponding position. If we decide to revisit the idea of using document fragments, it will be very easy to handle the `!== 1` case by returning a document fragment.
Ben Newman committed
Sep 3, 2013 at 18:16 UTC
dea0cc01cf92af4ee7966130f1a6d01203655442
2 files changed
+113
-25
src/dom/Danger.js
+98
-20
@@ -28,6 +28,8 @@ var emptyFunction = require('emptyFunction');
28
var getMarkupWrap = require('getMarkupWrap');
29
var invariant = require('invariant');
30
31
+var COMMENT_NODE_TYPE = 8;
32
+
33
/**
34
* Extracts the `nodeName` from a string of markup.
35
*
@@ -46,9 +48,9 @@ function getNodeName(markup) {
48
var Danger = {
49
50
/**
49
- * Renders markup into an array of nodes. The markup is expected to render
50
- * into a list of root nodes. Also, the length of `parentNodes` and `markup`
51
- * should be the same.
51
+ * Renders markup into an array of nodes. The markup is expected to
52
+ * render into a list of root nodes. Also, the length of `resultList`
53
+ * and `markupList` should be the same.
54
*
55
* @param {array<string>} markupList List of markup strings to render.
56
* @return {array<DOMElement>} List of rendered nodes.
@@ -75,33 +77,109 @@ var Danger = {
77
markupByNodeName[nodeName][i] = markupList[i];
78
}
79
var resultList = [];
80
+ var resultListAssignmentCount = 0;
81
for (nodeName in markupByNodeName) {
82
if (!markupByNodeName.hasOwnProperty(nodeName)) {
83
continue;
84
}
85
var markupListByNodeName = markupByNodeName[nodeName];
83
- var markup = markupListByNodeName.join('');
84
- // Render each group of markup with similar `nodeName`.
85
- var renderNodes = createNodesFromMarkup(markup, emptyFunction);
86
- // Restore the initial ordering.
87
- var renderIndex = 0;
88
- var resultIndex = 0;
89
- while (resultIndex < markupListByNodeName.length) {
90
- // `markupListByNodeName` may be a sparse array.
91
- if (markupListByNodeName[resultIndex]) {
92
- invariant(
93
- resultList[resultIndex] = renderNodes[renderIndex],
94
- 'dangerouslyRenderMarkup(...): Missing node.'
86
+
87
+ var commentedMarkupList = [];
88
+ // This for-in loop skips the holes of the sparse array. The order
89
+ // of iteration should follow the order of assignment, which happens
90
+ // to match numerical index order, but we don't rely on that.
91
+ for (var resultIndex in markupListByNodeName) {
92
+ if (markupListByNodeName.hasOwnProperty(resultIndex)) {
93
+ // Push the requested markup followed by a sentinel HTML
94
+ // comment. Comments are a good choice for sentinels because
95
+ // they can appear as children of any HTML node.
96
+ commentedMarkupList.push(
97
+ markupListByNodeName[resultIndex],
98
+ // This resultIndex will be parsed back out below.
99
+ '<!--danger:', resultIndex, '-->'
100
);
96
- renderIndex++;
101
}
98
- resultIndex++;
102
}
100
- invariant(
101
- renderIndex === renderNodes.length,
102
- 'dangerouslyRenderMarkupO(...): Unexpected nodes.'
103
+
104
+ // Render each group of markup with similar wrapping `nodeName`,
105
+ // with sentinel comments interspersed.
106
+ var renderNodes = createNodesFromMarkup(
107
+ commentedMarkupList.join(''),
108
+ emptyFunction // Do nothing special with <script> tags.
109
);
110
+
111
+ var renderBuffer = [];
112
+
113
+ for (i = 0; i < renderNodes.length; ++i) {
114
+ var renderNode = renderNodes[i];
115
+ if (renderNode.nodeType !== COMMENT_NODE_TYPE) {
116
+ // Append all non-comment nodes to the buffer.
117
+ renderBuffer.push(renderNode);
118
+
119
+ } else {
120
+ var comment = renderNode.nodeValue.split(':');
121
+ if (comment[0] === 'danger') {
122
+ resultIndex = comment[1];
123
+ invariant(
124
+ !resultList.hasOwnProperty(resultIndex),
125
+ 'Danger: Assigning to an already-occupied result index.'
126
+ );
127
+
128
+ // Take the first node no matter how many were rendered.
129
+ resultList[resultIndex] = renderBuffer[0] || null;
130
+
131
+ if (__DEV__) {
132
+ if (renderBuffer.length !== 1) {
133
+ var args = [
134
+ "Danger: '" + markupListByNodeName[resultIndex] + "' " +
135
+ "rendered as " + renderBuffer.length + " nodes instead of 1:"
136
+ ];
137
+
138
+ // Pass the nodes as arguments to console.error so that
139
+ // they can be inspected in the browser.
140
+ args.push.apply(args, renderBuffer);
141
+
142
+ // Use console.error instead of throwing so that we don't
143
+ // interfere with the rendering of other markup.
144
+ console.error.apply(console, args);
145
+ }
146
+ }
147
+
148
+ // Empty the buffer after assigning its contents to
149
+ // resultList[resultIndex];
150
+ renderBuffer.length = 0;
151
+
152
+ // This should match resultList.length and markupList.length
153
+ // when we're done.
154
+ resultListAssignmentCount += 1;
155
+
156
+ } else {
157
+ // It's some other kind of comment that we didn't create.
158
+ // Weirder things have happened.
159
+ renderBuffer.push(renderNode);
160
+ console.warn(
161
+ 'Danger: Markup unexpectedly rendered a comment node:',
162
+ '<!--' + renderNode.nodeValue + '-->.'
163
+ );
164
+ }
165
+ }
166
+ }
167
}
168
+
169
+ // Although resultList was populated out of order, it should now be a
170
+ // dense array.
171
+ invariant(
172
+ resultListAssignmentCount === resultList.length,
173
+ 'Danger: Did not assign to every index of resultList.'
174
+ );
175
+
176
+ invariant(
177
+ resultList.length === markupList.length,
178
+ 'Danger: Expected markup to render %d nodes, but rendered %d.',
179
+ markupList.length,
180
+ resultList.length
181
+ );
182
+
183
return resultList;
184
},
185
src/dom/__tests__/Danger-test.js
+15
-5
@@ -96,11 +96,21 @@ describe('Danger', function() {
96
'Invariant Violation: dangerouslyRenderMarkup(...): Missing markup.'
97
);
98
99
- expect(function() {
100
- Danger.dangerouslyRenderMarkup(['<p></p><p></p>']);
101
- }).toThrow(
102
- 'Invariant Violation: dangerouslyRenderMarkupO(...): Unexpected nodes.'
103
- );
99
+ spyOn(console, "error");
100
+
101
+ var renderedMarkup = Danger.dangerouslyRenderMarkup(['<p></p><p></p>']);
102
+ var args = console.error.argsForCall[0];
103
+
104
+ expect(renderedMarkup.length).toBe(1);
105
+ expect(renderedMarkup[0].nodeName).toBe('P');
106
+
107
+ expect(console.error.argsForCall.length).toBe(1);
108
+
109
+ expect(args.length).toBe(3);
110
+ expect(args[0]).toBe(
111
+ "Danger: '<p></p><p></p>' rendered as 2 nodes instead of 1:");
112
+ expect(args[1]).toBe(renderedMarkup[0]);
113
+ expect(args[2].nodeName).toBe('P');
114
});
115
});
116