Merge pull request #182 from declandewet/nested-html-fix

fatfisz · web-flow · commit 20cd1fd3d0b4 · 2019-01-20T22:16:38.000+01:00
Fix the case of nested code for the source tag
diff --git a/src/html/fixtures/empty-array-multiline.txt b/src/html/fixtures/empty-array-multiline.txt
@@ -0,0 +1,8 @@
+<!DOCTYPE html>
+<html lang="en">
+  <body>
+    <ul>
+
+    </ul>
+  </body>
+</html>
diff --git a/src/html/fixtures/empty-array.txt b/src/html/fixtures/empty-array.txt
@@ -0,0 +1,6 @@
+<!DOCTYPE html>
+<html lang="en">
+  <body>
+    <ul></ul>
+  </body>
+</html>
diff --git a/src/html/fixtures/nesting-improper.txt b/src/html/fixtures/nesting-improper.txt
@@ -0,0 +1,12 @@
+    <!DOCTYPE html>
+    <html lang="en">
+      <body>
+        <ul><li>
+  <div>apple</div>
+</li> <li>
+  <div>banana</div>
+</li> <li>
+  <div>kiwi</div>
+</li></ul>
+      </body>
+    </html>
diff --git a/src/html/fixtures/nesting-no-excess.txt b/src/html/fixtures/nesting-no-excess.txt
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html lang="en">
+  <body>
+    <ul>
+
+      <li>
+        <div>apple</div>
+      </li>
+      <li>
+        <div>banana</div>
+      </li>
+      <li>
+        <div>kiwi</div>
+      </li>
+
+    </ul>
+  </body>
+</html>
diff --git a/src/html/fixtures/nesting.txt b/src/html/fixtures/nesting.txt
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<html lang="en">
+  <body>
+    <ul>
+      <li>
+        <div>apple</div>
+      </li>
+      <li>
+        <div>banana</div>
+      </li>
+      <li>
+        <div>kiwi</div>
+      </li>
+    </ul>
+  </body>
+</html>
diff --git a/src/html/html.test.js b/src/html/html.test.js
@@ -45,3 +45,111 @@ test('does not introduce excess newlines', () => {
   `;
   expect(actual).toBe(expected);
 });
+
+test('renders nested HTML', () => {
+  const fruits = ['apple', 'banana', 'kiwi'];
+  const expected = readFromFixture(__dirname, 'nesting');
+
+  function renderFruit(fruit) {
+    return html`
+      <li>
+        <div>${fruit}</div>
+      </li>
+    `;
+  }
+
+  const actual = html`
+    <!DOCTYPE html>
+    <html lang="en">
+      <body>
+        <ul>
+          ${fruits.map(renderFruit)}
+        </ul>
+      </body>
+    </html>
+  `;
+
+  expect(actual).toBe(expected);
+});
+
+test('renders nested HTML without excess empty lines', () => {
+  const fruits = ['apple', 'banana', 'kiwi'];
+  const expected = readFromFixture(__dirname, 'nesting-no-excess');
+
+  function renderFruit(fruit) {
+    return html`
+      <li>
+        <div>${fruit}</div>
+      </li>
+    `;
+  }
+
+  const actual = html`
+    <!DOCTYPE html>
+    <html lang="en">
+      <body>
+        <ul>
+
+          ${fruits.map(renderFruit)}
+
+        </ul>
+      </body>
+    </html>
+  `;
+
+  expect(actual).toBe(expected);
+});
+
+test("just strips indent when there's an empty array inside", () => {
+  const expected = readFromFixture(__dirname, 'empty-array');
+  const actual = html`
+    <!DOCTYPE html>
+    <html lang="en">
+      <body>
+        <ul>${[]}</ul>
+      </body>
+    </html>
+  `;
+
+  expect(actual).toBe(expected);
+});
+
+test("just strips indent when there's an empty array inside (multiline)", () => {
+  const expected = readFromFixture(__dirname, 'empty-array-multiline');
+  const actual = html`
+    <!DOCTYPE html>
+    <html lang="en">
+      <body>
+        <ul>
+          ${[]}
+        </ul>
+      </body>
+    </html>
+  `;
+
+  expect(actual).toBe(expected);
+});
+
+test('may not indent as expected when the array is not in a new line', () => {
+  const fruits = ['apple', 'banana', 'kiwi'];
+  const expected = readFromFixture(__dirname, 'nesting-improper');
+
+  function renderFruit(fruit) {
+    return html`
+      <li>
+        <div>${fruit}</div>
+      </li>
+    `;
+  }
+
+  const actual = html`
+    <!DOCTYPE html>
+    <html lang="en">
+      <body>
+        <ul>${fruits.map(renderFruit)}</ul>
+      </body>
+    </html>
+  `;
+
+  expect(actual).toBe(expected);
+});
diff --git a/src/inlineArrayTransformer/inlineArrayTransformer.js b/src/inlineArrayTransformer/inlineArrayTransformer.js
@@ -1,10 +1,12 @@
+import { prefixLines, stripLastNewLine } from '../utils';
+
 /**
  * Converts an array substitution to a string containing a list
- * @param  {String} [opts.separator = ''] - the character that separates each item
- * @param  {String} [opts.conjunction = '']  - replace the last separator with this
- * @param  {Boolean} [opts.serial = false] - include the separator before the conjunction? (Oxford comma use-case)
+ * @param {String} [opts.separator = '']   - The character that separates each item
+ * @param {String} [opts.conjunction = ''] - Replace the last separator with this
+ * @param {Boolean} [opts.serial = false]  - Include the separator before the conjunction? (Oxford comma use-case)
  *
- * @return {Object}                     - a TemplateTag transformer
+ * @return {Object}                        - A transformer
  */
 const inlineArrayTransformer = ({
   conjunction = '',
@@ -17,20 +19,30 @@ const inlineArrayTransformer = ({
       return substitution;
     }
 
-    // be sure to maintain indentation
-    const indent = resultSoFar.match(/(\n?[^\S\n]+)$/);
-    const fullSeparator = separator.concat(indent ? indent[1] : ' ');
-    const fullConjunction = ''.concat(conjunction, ' ');
-    const conjunctionIndex = conjunction ? substitution.length - 1 : -1;
+    const { length } = substitution;
+    const lastSeparatorIndex = conjunction && !serial ? length - 2 : length - 1;
+    const indentation = resultSoFar.match(/(?:\n)([^\S\n]+)$/);
+
+    if (conjunction && length > 1) {
+      substitution[length - 1] = ''.concat(
+        conjunction,
+        ' ',
+        substitution[length - 1],
+      );
+    }
 
-    return substitution.reduce((result, part, index) =>
-      ''.concat(
+    return substitution.reduce((result, part, index) => {
+      const isFirstPart = index === 0;
+      const strippedPart = stripLastNewLine(part);
+      return ''.concat(
         result,
-        index !== conjunctionIndex || serial ? fullSeparator : ' ',
-        index === conjunctionIndex ? fullConjunction : '',
-        part,
-      ),
-    );
+        isFirstPart ? '' : indentation ? '\n' : ' ',
+        indentation
+          ? prefixLines(indentation[1], strippedPart, isFirstPart)
+          : strippedPart,
+        index < lastSeparatorIndex ? separator : '',
+      );
+    }, '');
   },
 });
 
diff --git a/src/inlineArrayTransformer/inlineArrayTransformer.test.js b/src/inlineArrayTransformer/inlineArrayTransformer.test.js
@@ -62,6 +62,38 @@ test('maintains indentation', () => {
   );
 });
 
+test('maintains indentation in multiline strings', () => {
+  const tag = createTag(inlineArrayTransformer({ separator: ',' }));
+  expect(tag`My friends are always
+  ${['dra-\nmatic', 'emo-\ntional', 'nee-\ndy']}`).toBe(
+    'My friends are always\n  dra-\n  matic,\n  emo-\n  tional,\n  nee-\n  dy',
+  );
+});
+
+test('maintains indentation in multiline strings (with conjunction)', () => {
+  const tag = createTag(
+    inlineArrayTransformer({ separator: ',', conjunction: 'and' }),
+  );
+  expect(tag`My friends are always
+  ${['dra-\nmatic', 'emo-\ntional', 'nee-\ndy']}`).toBe(
+    'My friends are always\n  dra-\n  matic,\n  emo-\n  tional\n  and nee-\n  dy',
+  );
+});
+
+test('maintains indentation in multiline strings (with serial/oxford separators)', () => {
+  const tag = createTag(
+    inlineArrayTransformer({
+      separator: ',',
+      conjunction: 'and',
+      serial: true,
+    }),
+  );
+  expect(tag`My friends are always
+  ${['dra-\nmatic', 'emo-\ntional', 'nee-\ndy']}`).toBe(
+    'My friends are always\n  dra-\n  matic,\n  emo-\n  tional,\n  and nee-\n  dy',
+  );
+});
+
 test('does not introduce excess newlines', () => {
   const tag = createTag(inlineArrayTransformer());
   expect(tag`My friends are always
diff --git a/src/utils/index.js b/src/utils/index.js
@@ -1 +1,4 @@
 export flat from './flat';
+export prefixLines from './prefixLines';
+export stripLastNewLine from './stripLastNewLine';
+export toString from './toString';
diff --git a/src/utils/prefixLines/index.js b/src/utils/prefixLines/index.js
@@ -0,0 +1 @@
+export default from './prefixLines';
diff --git a/src/utils/prefixLines/prefixLines.js b/src/utils/prefixLines/prefixLines.js
@@ -0,0 +1,11 @@
+import toString from '../toString';
+
+export default function prefixLines(prefix, value, skipFirst = false) {
+  return toString(value)
+    .split('\n')
+    .map(
+      (line, index) =>
+        skipFirst && index === 0 ? line : ''.concat(prefix, line),
+    )
+    .join('\n');
+}
diff --git a/src/utils/prefixLines/prefixLines.test.js b/src/utils/prefixLines/prefixLines.test.js
@@ -0,0 +1,22 @@
+import prefixLines from './prefixLines';
+
+test('it should prefix lines with a given string', () => {
+  const actual = prefixLines('foo', '  a  \n  b  \n  c  ');
+  const expected = 'foo  a  \nfoo  b  \nfoo  c  ';
+
+  expect(actual).toEqual(expected);
+});
+
+test('it should prefix lines with a given non-string value', () => {
+  const actual = prefixLines(42, '  a  \n  b  \n  c  ');
+  const expected = '42  a  \n42  b  \n42  c  ';
+
+  expect(actual).toEqual(expected);
+});
+
+test('it should skip the first line with the option set', () => {
+  const actual = prefixLines('foo', '  a  \n  b  \n  c  ', true);
+  const expected = '  a  \nfoo  b  \nfoo  c  ';
+
+  expect(actual).toEqual(expected);
+});
diff --git a/src/utils/stripLastNewLine/index.js b/src/utils/stripLastNewLine/index.js
@@ -0,0 +1 @@
+export default from './stripLastNewLine';
diff --git a/src/utils/stripLastNewLine/stripLastNewLine.js b/src/utils/stripLastNewLine/stripLastNewLine.js
@@ -0,0 +1,9 @@
+import toString from '../toString';
+
+export default function stripLastNewLine(value) {
+  const stringValue = toString(value);
+  const { length } = stringValue;
+  return length > 0 && stringValue[length - 1] === '\n'
+    ? stringValue.slice(0, length - 1)
+    : stringValue;
+}
diff --git a/src/utils/stripLastNewLine/stripLastNewLine.test.js b/src/utils/stripLastNewLine/stripLastNewLine.test.js
@@ -0,0 +1,22 @@
+import stripLastNewLine from './stripLastNewLine';
+
+test('it should strip last new line with a given string', () => {
+  const actual = stripLastNewLine('foo\n');
+  const expected = 'foo';
+
+  expect(actual).toEqual(expected);
+});
+
+test('it should leave a string as is when the last character is not a new line', () => {
+  const actual = stripLastNewLine('foo');
+  const expected = 'foo';
+
+  expect(actual).toEqual(expected);
+});
+
+test('it should return an empty string for an empty string', () => {
+  const actual = stripLastNewLine('');
+  const expected = '';
+
+  expect(actual).toEqual(expected);
+});
diff --git a/src/utils/toString/index.js b/src/utils/toString/index.js
@@ -0,0 +1 @@
+export default from './toString';
diff --git a/src/utils/toString/toString.js b/src/utils/toString/toString.js
@@ -0,0 +1,6 @@
+export default function toString(value) {
+  // Use concat rather than string so that the behavior is the same as when
+  // expressions are evaluated in templates (look for Runtime Semantics:
+  // Evaluation of a TemplateLiteral).
+  return ''.concat(value);
+}
diff --git a/src/utils/toString/toString.test.js b/src/utils/toString/toString.test.js
@@ -0,0 +1,22 @@
+import toString from './toString';
+
+test('transforms values to string as per spec', () => {
+  const get = jest
+    .fn()
+    .mockImplementationOnce((target, prop) => {
+      expect(prop).toBe(Symbol.toPrimitive);
+    })
+    .mockImplementationOnce((target, prop) => {
+      expect(prop).toBe('toString');
+    })
+    .mockImplementationOnce((target, prop) => {
+      expect(prop).toBe('valueOf');
+      return () => 42;
+    });
+
+  const val = new Proxy({}, { get });
+  const result = toString(val);
+
+  expect(get).toHaveBeenCalledTimes(3);
+  expect(result).toBe('42');
+});

-Original file line number
+Diff line change
@@ @@ -0,0 +1,8 @@ @@
 +<!DOCTYPE html>
 +<html lang="en">
 +  <body>
 +    <ul>
++
 +    </ul>
 +  </body>
 +</html>