Discuss edge cases of indentation in pretty-format plugins

[pedrottimark]

Do you want to request a feature or report a bug?

Report bugs with indentation in ReactElement and ReactTestComponent plugins

What is the current behavior?

The first problem I found and put aside while writing test cases for #3429 notice that the second and third lines of multiline string are indented for points prop. If we change the hypothetical component to render a g element in the tree between svg and polyline then the additional level of indentation within a multiline string will defeat ignoring indentation for the points prop and it will appear in the diff as removed and added :(

<svg>
  <desc>
    JavaScript community logo as a stick figure
  </desc>
  <polyline
    id="J"
    points="0.5,0.460
  0.5,0.875
  0.25,0.875"
  />
</svg>

Compare to pretty-format for a similar object which respects the lack of indentation:

Object {
  "id": "J",
  "points": "0.5,0.460
0.5,0.875
0.25,0.875",
}

The second problem I found while studying the code in pretty-format and its plugins. It might become a more than theoretical problem when React components can render fragments.

• The plugins call the bound indent function as if it represents one indent level. That assumption isn’t true for an array of elements (or for elements which are props of elements).
• Also notice no indentation of the start tag close or the end tag because the code assumes that it is always at the margin.

For a minimal example of array of elements, see:
https://github.com/adriantoine/enzyme-to-json/blob/master/tests/core/__snapshots__/mount.test.js.snap#L222-L231

In the following example, notice how the indent level changes from 2 to 4 starting from the prop
because the total indentation at that point is used incorrectly as one level increment.

Array [
  <div
    id="A"
>
    text of A
</div>,
  <div
    id="B"
    prop={
        <Parent
            propBoolean={true}
            propString="valueA"
        >
            <Child
                propArray={
                    Array [
                        0,
                        1,
                        2,
                      ]
                }
            >
                text of child
            </Child>
        </Parent>
    }
>
    text of B
</div>,
  <div
    id="C"
>
    text of C
</div>,
]

EDIT on 2017-07-11 while studying #1772 found excess indentation in prop of React 15 component instance:

          "_currentElement": <TopLevelWrapper
            child={
                    <MyComponent
                            className="same"
                            text="expected"
                    />
            }
    />,

What is the expected behavior?

Plugins (especially for React elements and test objects) return indentation that is consistent with pretty-format for similar data structures (for example, objects).

EDIT on 2017-07-11 disregard the following and skip to comments at the end of the issue :)

I am happy to receive your critique about an idea how to solve the problem. Provide as options to plugins two functions (please, please, please suggest better names than the following :-)

• returnIndentationString(nLevels) returns the current indentation plus nLevels additional indent levels:
  • nLevels = 0: element start tag open <type or close > or end tag </type>
  • nLevels = 1: prop key prop="value" or prop={
• returnPrintFunction(nLevels) returns a print function that is bound to the current indentation plus nLevels additional indent levels:
  • nLevels = 2: multiline prop values enclosed in braces
  • nLevels = 1: children

Jest 19.0.2 and Node 7.10.0

Depending on your feedback, I can create a sample project for Jest 20 and take a baseline perf profile for a few examples of the unexpected indentation. It seems like calling a print function that is bound with the correct indentation should outperform calling indent to replace the (sometimes incorrect) indentation afterward.

[thymikee]

Seems like a nice enhancement, although we need to keep in mind this is a breaking change.

[pedrottimark]

Yeah, for sure. Am interested to know if arrays of test objects might become more common if render method can return arrays of elements in React 16? So far, has been from enzyme query.

There is some irony in the digital archaeology or cultural anthropology:

• 3.2.0: all my stress tests work correctly in the first version with plugin for test objects :)
• 3.3.0: multiline strings as prop values of test objects over-indented :(
  • comment thread: ReactTestRenderer plugin first version jamiebuilds/pretty-format#10 (comment)
  • pull request: Refactor react plugin jamiebuilds/pretty-format#11
• 3.4.0 - 3.5.0: only tests plugins at the top level, so hides the other problems
• 3.5.1: tests plugins at lower levels, so you can see the other problems :(
  • under-indented end tags of test objects in an array
  • over-indented props or children child elements
  • over-indented element props of elements not at top level
• 3.7.0: React element plugin has same problems for same reasons

[pedrottimark]

Sample of proposed changes to pretty-format/src/index.js so it and plugins indent consistently.

Have adapted perf/test.js and relevant parts of React-test.js plus a few of my stress tests. Need to take a break and get your feedback before I study other plugins than React.

The limiting factor to move forward is whether to adjust the plugin API:

• baseline: return plugin.print(val, boundPrint, boundIndent, opts, colors);
• proposed: return plugin.print(val, printer, indenter, level, opts, colors); see below

My first attempt a few weeks ago to leave args the same and add properties to opts ran slower than baseline, I guess because of the Object.assign calls and bound function closures.

Proposed version runs faster in many cases and is within measurement precision in the rest :)

internal function arguments

• baseline: indent: string, prevIndent: string
• proposed: indenter: Indenter, prevLevel: number

assignment statements

• baseline: const innerIndent = prevIndent + indent;
• proposed: const nextLevel = prevLevel + 1; const innerIndent = indenter(nextLevel);

initialization function, which is not called for basic values

• createIndent(indent: number): string { return new Array(indent + 1).join(' '); }
• return a function which caches indentations, as follows:

type Indenter = (level: number) => string;

function createIndenter(indent: number): Indenter {
  if (indent === 0) {
    return function () {
      return '';
    }
  }

  let indentation = ['', new Array(indent + 1).join(' ')];

  // Given number of indent levels, return indentation string.
  return function (n) {
    for (let i = indentation.length; i <= n; i += 1) {
      indentation[i] = indentation[i - 1] + indentation[1];
    }
    return indentation[n];
  }
}

bound print function for plugins

• baseline: function boundPrint(val) { return print(val, indent, prevIndent, …); }
• proposed: function printer(val, level) { return print(val, indenter, level, …); }

bound indent function for plugins

• baseline: function boundIndent(str) { … }
• proposed: indenter returned from createIndenter(indent) see above

[pedrottimark]

Forgot to say that a way forward is support another key in plugin object for proposed arguments:

• upgrade the built-in plugins
• to support external plugins, fall back to call baseline print method if new key not in object

[pedrottimark]

It has turned out as Fred Brooks advised: plan to throw one away; you will, anyhow.

• After I wrote a memoized version described in the preceding comment, I had second thoughts about garbage collection of indentation array.
• So I wrote a version that applies the indentation pattern from pretty-format core to plugins, so you can learn once, serialize anything :)

Measurements of 22 tests adapted from React-test.js plus 3 stress tests for previously incorrect indentation. Each value is a geometric mean for 3 runs of tests (each run after a restart and unconnected to network).

scenario	mean	ratio
baseline	1122281	1.000
memoized	932805	0.831
proposed	905215	0.807

Measurements of 22 tests for hypothesis in #3745 that proposed version will make up the time lost from the plugin shortcut, because it replaces regexp with concatenation.

scenario	mean	ratio
baseline before 3745	679633	1.000
baseline after 3745	702370	1.033
proposed	580285	0.854

Bound print function for plugins

• baseline: function boundPrint(val) { return print(val, indent, prevIndent, …); }
• proposed: function boundPrint(val, argIndent) { return print(val, indent, argIndent, …); }

Bound indent function for plugins

• baseline: function boundIndent(str) { … }
• proposed: replaced by indent and prevIndent args (therefore, one less closure :)

Plugin API:

• baseline: return plugin.print(val, boundPrint, boundIndent, opts, colors);
• proposed: return plugin.WHAT(val, boundPrint, indent, prevIndent, opts, colors);

Both alternatives add an arg so plugin has the indentation. That was the root of the problem.

Y’all have given such super direction about API for my pull requests.

So WHAT do you suggest as an object key for serializers which support the proposed API?

function printPlugin(val, indent, prevIndent, /* and so on */) {}
  // Find plugin whose test matches val.

  if (typeof plugin.WHAT !== 'function') {
    // Old API
    function boundPrint(val) {
      return print(val, indent, prevIndent, /* and so on */);
    }
    function boundIndent(str) {
      const indentation = prevIndent + indent;
      return indentation + str.replace(NEWLINE_REGEXP, '\n' + indentation);
    }
    return plugin.print(val, boundPrint, boundIndent, opts, colors);
  }

  // New API
  function boundPrint(val, argIndent) {
    return print(val, indent, argIndent, /* and so on */);
  }
  return plugin.WHAT(val, boundPrint, indent, prevIndent, opts, colors);
}

[pedrottimark]

What do you think about serialize as property name for the new API?

Because we have already merged breaking changes for edge cases of HTMLElement plugin, how about getting ahead of the current incorrect indentation which seems like it could affect snapshots when render function can return arrays of elements in React 16?

Here is some pseudo Flow.

export type PrintOld = any => string;
export type PrintNew = (val: any, indentation: string) => string;

export type Plugin = {
  print: (
    val: any,
    print: PrintOld,
    indent: Indent,
    opts: PluginOptions,
    colors: Colors,
  ) => string,
  serialize: (
    val: any,
    print: PrintNew,
    indentation: string, // current indentation
    indent: string, // string of spaces whose length is specified by indent number option
    opts: PluginOptions,
    colors: Colors,
  ) => string,
  test: any => boolean,
};

P.S. Thinking about adding an if statement to support old versus new API, I became worried that bound function declarations are “hoisted” within printPlugin to precede the existing if statement, see https://github.com/facebook/jest/blob/master/packages/pretty-format/src/index.js#L698-L724

Indeed, even with no plugins at all, look at the “massive” world.geo.json perf test case!

bound functions	time	ratio
baseline function declaration	70596180	1.0000
const initialized to function expression	46350463	0.6565
arrow function as arg	46068780	0.6526

[pedrottimark]

Maybe this issue becomes “slow good” like a Heinz ketchup advert. Here’s a way to make API of (and code snippets in) plugins very similar to pretty-format core functions:

• indentation, depth, refs arguments depend on location of val in tree structure
• config object depends only on initial options or defaults

Plugin property:

• baseline: print(val, printer, indenter, opts, colors)
• proposed: serialize(val, config, printer, indentation, depth, refs)

Plugin callback arg to print descendants:

• baseline: printer(val) is bound to the rest of the “state” of the current node
• proposed: printer(val, indentation, depth, refs) is bound to unchanging config and plugin provides indentation for the descendant. It might make sense to allow depth or refs to default to the “state” of the current node if the caller leaves either argument undefined

Plugin object args:

• baseline: colors and opts which has edgeSpacing, min, spacing properties
• proposed: config which has callToJSON, colors, escapeRegex, indent: string, maxDepth, min, plugins, printFunctionName, spacingInner replaces spacing and still depends on min, spacingOuter replaces edgeSpacing and still depends on min

Example of a major core function:

• baseline: printComplexValue(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors)
• proposed: printComplex(val, config, indentation, depth, refs)

Example of minor core function:

• baseline: printList(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors)
• proposed: printList(val, config, indentation, depth, refs)

As suggested in preceding comments, we can:

• upgrade built-in plugins to new serialize property
• support print property in external plugins for as long as needed

Performance tests suggest that:

• config object does not slow things down
• there is some room for improving lazy conversion from initial options (if any) to config especially for the use case outside Jest of printing a basic value with no plugins.