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

Add Flow type annotations to pretty-format #2977

Merged
merged 5 commits into from
Feb 28, 2017
Merged

Add Flow type annotations to pretty-format #2977

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45e77e6
Add Flow type annotations to pretty-format
pedrottimark Feb 22, 2017
6c12642
Move new test to be among existing tests
pedrottimark Feb 22, 2017
3827a5d
modify test of properties and symbols to emphasize correct sorting
pedrottimark Feb 22, 2017
78dd416
exact type for theme and if plugin returns string
pedrottimark Feb 27, 2017
0038d9f
rebase to include changes from 3017
pedrottimark Feb 27, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 19 additions & 1 deletion packages/pretty-format/src/__tests__/pretty-format-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,10 @@ describe('prettyFormat()', () => {
});

it('prints an object with properties and symbols', () => {
const val = {prop: 'value1'};
const val = {};
val[Symbol('symbol1')] = 'value2';
val[Symbol('symbol2')] = 'value3';
val.prop = 'value1';
expect(prettyFormat(val)).toEqual('Object {\n "prop": "value1",\n Symbol(symbol1): "value2",\n Symbol(symbol2): "value3",\n}');
});

Expand Down Expand Up @@ -303,6 +304,23 @@ describe('prettyFormat()', () => {
})).toEqual('class Foo');
});

it('supports plugins that return empty string', () => {
const val = {
payload: '',
};
const options = {
plugins: [{
print(val) {
return val.payload;
},
test(val) {
return val && typeof val.payload === 'string';
},
}],
};
expect(prettyFormat(val, options)).toEqual('');
});

it('supports plugins with deeply nested arrays (#24)', () => {
const val = [[1, 2], [3, 4]];
expect(prettyFormat(val, {
Expand Down
129 changes: 90 additions & 39 deletions packages/pretty-format/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @flow
*/
/* eslint-disable max-len */

Expand All @@ -22,7 +24,7 @@ const NEWLINE_REGEXP = /\n/ig;

const getSymbols = Object.getOwnPropertySymbols || (obj => []);

function isToStringedArrayType(toStringed) {
function isToStringedArrayType(toStringed: string): boolean {
return (
toStringed === '[object Array]' ||
toStringed === '[object ArrayBuffer]' ||
Expand All @@ -39,15 +41,15 @@ function isToStringedArrayType(toStringed) {
);
}

function printNumber(val) {
function printNumber(val: number): string {
if (val != +val) {
return 'NaN';
}
const isNegativeZero = val === 0 && (1 / val) < 0;
return isNegativeZero ? '-0' : '' + val;
}

function printFunction(val, printFunctionName) {
function printFunction(val: Function, printFunctionName: boolean): string {
if (!printFunctionName) {
return '[Function]';
} else if (val.name === '') {
Expand All @@ -57,15 +59,15 @@ function printFunction(val, printFunctionName) {
}
}

function printSymbol(val) {
function printSymbol(val: Symbol): string {
return symbolToString.call(val).replace(SYMBOL_REGEXP, 'Symbol($1)');
}

function printError(val) {
function printError(val: Error): string {
return '[' + errorToString.call(val) + ']';
}

function printBasicValue(val, printFunctionName, escapeRegex) {
function printBasicValue(val: any, printFunctionName: boolean, escapeRegex: boolean): StringOrNull {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use mixed instead of any. any == unsafe, mixed == unknown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thank you, in all 8 occurrences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uh oh, just for the one change, 10 errors including:

function printBasicValue(val: mixed, printFunctionName: boolean, escapeRegex: boolean): StringOrNull {
  if (val === true || val === false) {
    return '' + val;
  }
packages/pretty-format/src/index.js:72
 72:     return '' + val;
                     ^^^ boolean literal `false`. This type cannot be added to
 72:     return '' + val;
                ^^ string

packages/pretty-format/src/index.js:72
 72:     return '' + val;
                     ^^^ boolean literal `true`. This type cannot be added to
 72:     return '' + val;
                ^^ string
  const typeOf = typeof val;

  if (typeOf === 'number') {
    return printNumber(val);
  }
packages/pretty-format/src/index.js:84
 84:     return printNumber(val);
                            ^^^ mixed. This type is incompatible with the expected param type of
 44: function printNumber(val: number): string {
                               ^^^^^^ number

Copy link
Contributor

@jamiebuilds jamiebuilds Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, this is what I would've expected. When I was writing pretty-format I did all sorts of micro-optimizations like storing typeof val as a variable (which doesn't work as a refinement in Flow). Flow also requires things to be stringified in a certain way (i.e. '' + bool is not considered valid).

We don't really want to modify the code of pretty-format by much because the optimizations do matter to make it as fast as JSON.stringify.

The only remaining option would be to type cast values in different places:

const typeOf = typeof val;

if (typeOf === 'number') { 
  return printNumber(((val: any): number));
}
if (val === true || val === false) {
  return '' + ((val: any): string);
}

These will then get stripped away, leaving the original code.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thank you, in all 8 occurrences?

Yeah, you should try as much as possible to never use any. It's as an opt-out from Flow's type checking. You should only use it in scenarios like above where Flow can't do what you need it to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, you explain mixed vs any clearly. So, for code that’s carefully written, rarely edited, and perf critical, we might leave well enough alone with any in this PR?

For context, Jest had a regression in a serializer plugin recently, fixed in #2943, which Flow type checking would have prevented. Follow up includes adding enough checking to pretty-format to protect against, for example, editing error in args or undefined return value if a printAmazingFeatureInES20xx function is added.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what you're asking 😕

if (val === true || val === false) {
return '' + val;
}
Expand Down Expand Up @@ -129,10 +131,10 @@ function printBasicValue(val, printFunctionName, escapeRegex) {
return printError(val);
}

return false;
return null;
}

function printList(list, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
function printList(list: any, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
let body = '';

if (list.length) {
Expand All @@ -154,15 +156,15 @@ function printList(list, indent, prevIndent, spacing, edgeSpacing, refs, maxDept
return '[' + body + ']';
}

function printArguments(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
function printArguments(val: any, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
return (min ? '' : 'Arguments ') + printList(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors);
}

function printArray(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
function printArray(val: any, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
return (min ? '' : val.constructor.name + ' ') + printList(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors);
}

function printMap(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
function printMap(val: Map<any, any>, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
let result = 'Map {';
const iterator = val.entries();
let current = iterator.next();
Expand Down Expand Up @@ -191,14 +193,15 @@ function printMap(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth,
return result + '}';
}

function printObject(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
function printObject(val: Object, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
const constructor = min ? '' : (val.constructor ? val.constructor.name + ' ' : 'Object ');
let result = constructor + '{';
let keys = Object.keys(val).sort();
const symbols = getSymbols(val);

if (symbols.length) {
keys = keys
// $FlowFixMe string literal `symbol`. This value is not a valid `typeof` return value
.filter(key => !(typeof key === 'symbol' || toString.call(key) === '[object Symbol]'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can safely remove this check: typeof key === 'symbol'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean that .filter(key => toString.call(key) !== '[object Symbol]') is enough?

I guess it is moving properties whose keys are symbols to follow the other.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yup that's what I mean. symbol is indeed not a valid typeof return value and toString.call(symbol) works on Node 4.
Unless it's for perf, cc: @cpojer

Copy link
Contributor Author

@pedrottimark pedrottimark Feb 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Aha, I thought I ran into this limitation of Flow before. It’s part of ES2015.

So the second part might be a fall-back? Notice:

  const typeOf = typeof val;
  if (typeOf === 'symbol') {
    return printSymbol(val);
  }

EDIT and also:

  if (toStringed === '[object Symbol]') {
    return printSymbol(val);
  }

EDIT Will wait for second opinion. Adjusted existing test to emphasize correct sorting :)

.concat(symbols);
}
Expand Down Expand Up @@ -226,7 +229,7 @@ function printObject(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDep
return result + '}';
}

function printSet(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
function printSet(val: Set<any>, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
let result = 'Set {';
const iterator = val.entries();
let current = iterator.next();
Expand All @@ -252,7 +255,7 @@ function printSet(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth,
return result + '}';
}

function printComplexValue(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
function printComplexValue(val: any, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
refs = refs.slice();
if (refs.indexOf(val) > -1) {
return '[Circular]';
Expand Down Expand Up @@ -282,21 +285,19 @@ function printComplexValue(val, indent, prevIndent, spacing, edgeSpacing, refs,
return hitMaxDepth ? '[Object]' : printObject(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors);
}

function printPlugin(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
let match = false;
function printPlugin(val, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): StringOrNull {
let plugin;

for (let p = 0; p < plugins.length; p++) {
plugin = plugins[p];

if (plugin.test(val)) {
match = true;
if (plugins[p].test(val)) {
plugin = plugins[p];
break;
}
}

if (!match) {
return false;
if (!plugin) {
return null;
}

function boundPrint(val) {
Expand All @@ -316,21 +317,67 @@ function printPlugin(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDep
return plugin.print(val, boundPrint, boundIndent, opts, colors);
}

function print(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors) {
const plugin = printPlugin(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors);
if (plugin) {
return plugin;
function print(val: any, indent: string, prevIndent: string, spacing: string, edgeSpacing: string, refs: Refs, maxDepth: number, currentDepth: number, plugins: Plugins, min: boolean, callToJSON: boolean, printFunctionName: boolean, escapeRegex: boolean, colors: Colors): string {
const pluginsResult = printPlugin(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors);
if (typeof pluginsResult === 'string') {
return pluginsResult;
}

const basic = printBasicValue(val, printFunctionName, escapeRegex);
if (basic) {
return basic;
const basicResult = printBasicValue(val, printFunctionName, escapeRegex);
if (basicResult !== null) {
return basicResult;
}

return printComplexValue(val, indent, prevIndent, spacing, edgeSpacing, refs, maxDepth, currentDepth, plugins, min, callToJSON, printFunctionName, escapeRegex, colors);
}

const DEFAULTS = {
type Colors = Object;
type Indent = (str: string) => string;
type Refs = Array<any>;
type Serialize = (val: any) => string;
type StringOrNull = string | null; // but disallow undefined, unlike ?string

type Plugin = {
print: (val: any, serialize: Serialize, indent: Indent, opts: Object, colors: Colors) => string,
test: Function,
}
type Plugins = Array<Plugin>;

type InitialOptions = {|
callToJSON?: boolean,
escapeRegex?: boolean,
highlight?: boolean,
indent?: number,
maxDepth?: number,
min?: boolean,
plugins?: Plugins,
printFunctionName?: boolean,
theme?: {
content: string,
prop: string,
tag: string,
value: string,
},
|};

type Options = {|
callToJSON: boolean,
escapeRegex: boolean,
highlight: boolean,
indent: number,
maxDepth: number,
min: boolean,
plugins: Plugins,
printFunctionName: boolean,
theme: {|
content: string,
prop: string,
tag: string,
value: string,
|},
|};

const DEFAULTS: Options = {
callToJSON: true,
escapeRegex: false,
highlight: false,
Expand All @@ -347,7 +394,7 @@ const DEFAULTS = {
},
};

function validateOptions(opts) {
function validateOptions(opts: InitialOptions) {
Object.keys(opts).forEach(key => {
if (!DEFAULTS.hasOwnProperty(key)) {
throw new Error('prettyFormat: Invalid option: ' + key);
Expand All @@ -359,7 +406,7 @@ function validateOptions(opts) {
}
}

function normalizeOptions(opts) {
function normalizeOptions(opts: InitialOptions): Options {
const result = {};

Object.keys(DEFAULTS).forEach(key =>
Expand All @@ -370,19 +417,23 @@ function normalizeOptions(opts) {
result.indent = 0;
}

return result;
// The type cast below means YOU are responsible to verify the code above.

// $FlowFixMe object literal. Inexact type is incompatible with exact type
return (result: Options);
}

function createIndent(indent) {
function createIndent(indent: number): string {
return new Array(indent + 1).join(' ');
}

function prettyFormat(val, opts) {
if (!opts) {
function prettyFormat(val: any, initialOptions?: InitialOptions): string {
let opts: Options;
if (!initialOptions) {
opts = DEFAULTS;
} else {
validateOptions(opts);
opts = normalizeOptions(opts);
validateOptions(initialOptions);
opts = normalizeOptions(initialOptions);
}

const colors = {};
Expand All @@ -405,13 +456,13 @@ function prettyFormat(val, opts) {
indent = createIndent(opts.indent);
refs = [];
const pluginsResult = printPlugin(val, indent, prevIndent, spacing, edgeSpacing, refs, opts.maxDepth, currentDepth, opts.plugins, opts.min, opts.callToJSON, opts.printFunctionName, opts.escapeRegex, colors);
if (pluginsResult) {
if (typeof pluginsResult === 'string') {
return pluginsResult;
}
}

const basicResult = printBasicValue(val, opts.printFunctionName, opts.escapeRegex);
if (basicResult) {
if (basicResult !== null) {
Copy link
Member

Choose a reason for hiding this comment

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

Should these checks really be !== null? Technically you are changing behavior here from what it was before. Should we do != null to also catch undefined here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I’m glad that you see and ask about this.

  • Because printBasicValue doesn’t return undefined or empty string, there’s no change in behavior for the internal code path, as verified by tests.

  • Because the 3 current built-in serializers don’t return those, no change there either.

  • The reason to recommend an intentional change in behavior now before many people write serializers, is to allow a serializer to return empty string.

    exports[`Allow user-defined serializer to return empty string 1`] = ``

    With current logic, print cannot distinguish no serializertest method matches from a serializer test method does match and its print method returns empty string.

    Although a rare edge case, I ran into the problem in first attempt at diff serializer, when it returned empty string for no differences between prev and next state.

  • Yes, I agree that != null is even better, in case a user-defined plugin returns undefined.

Copy link
Contributor Author

@pedrottimark pedrottimark Feb 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

On second thought, what do you think about:

  • if (basicResult !== null) for the internal trusted code, because Flow will find undefined and so on
  • if (typeof plugin === 'string') for the external untrusted code, in case a serializer returns anything other than a string. Does that test cause a performance problem?

return basicResult;
}

Expand Down