Add Flow type annotations to pretty-format #2977

pedrottimark · 2017-02-22T16:42:10Z

Summary

Add annotations to prevent problems when the code is edited to support new JavaScript features.

Reasons for minor changes to the code:

printPlugin: removed match because Flow thought that plugin could be undefined.
printBasicValue and printPlugin: return null instead of false because Flow had to consider the possibility of true value in if statement following the call.
print and prettyFormat: test explicitly not equal to null returned value to distinguish plugin not found from plugin returns empty string. Adding annotations confirmed subtle problem that I ran into a few months ago when initial attempt at a diff serializer returned empty string for no differences. Added a test to document expected behavior in this edge case.
prettyFormat: not assign to opts argument because Flow no like change of type from InitialOptions with optional properties to Options with required properties.

Only 2 $FlowFixMe comments:

normalizeOptions: type cast (result: Options)
printObject: typeof key === 'symbol'

Test plan

Type check and tests pass, including new test to verify an incorrect edge case is fixed.

I think Map<any, any> and Set<any> are okay for CI because Node 4 has basic support.

Future work

Colors type is unfinished because of edge cases with options in general and theme in specific.
If you agree with these complete annotations for function definitions to detect missing arguments or undefined return values, then I will complete the partial annotations in ReactTestComponent.

codecov-io · 2017-02-22T17:07:10Z

Codecov Report

Merging #2977 into master will decrease coverage by -0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2977      +/-   ##
==========================================
- Coverage   68.49%   68.47%   -0.02%     
==========================================
  Files         146      146              
  Lines        5335     5333       -2     
==========================================
- Hits         3654     3652       -2     
  Misses       1681     1681

Impacted Files	Coverage Δ
packages/pretty-format/src/index.js	`97.95% <100%> (-0.03%)`	❌

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c48a77...0038d9f. Read the comment docs.

thymikee · 2017-02-22T20:40:18Z

packages/pretty-format/src/index.js

  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]'))


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

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.

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

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

Support typeof x === "symbol" facebook/flow#1015

http://eslint.org/docs/rules/valid-typeof

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 :)

thymikee · 2017-02-22T20:41:18Z

👍 love your work!

cpojer · 2017-02-27T02:27:12Z

packages/pretty-format/src/index.js

      return pluginsResult;
    }
  }

  const basicResult = printBasicValue(val, opts.printFunctionName, opts.escapeRegex);
-  if (basicResult) {
+  if (basicResult !== null) {


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?

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.

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?

cpojer · 2017-02-27T02:28:02Z

packages/pretty-format/src/index.js

+  min: boolean,
+  plugins: Plugins,
+  printFunctionName: boolean,
+  theme: {


should this also use exact types?

Yes, good catch.

In the next pull request, normalizeOptions merges theme from options argument with default theme to fulfill the exact type. Now, an incomplete theme causes plugin to throw.

cpojer

Waiting for some comments by @pedrottimark

cpojer · 2017-02-27T15:15:00Z

@pedrottimark thanks for the replies. I'm unclear whether you were going to make some changes to the code though?

pedrottimark · 2017-02-27T15:23:44Z

Yes, next commit will have changes:

to exact type for theme in Options
to if(typeof pluginsResult === 'string') if you agree with that reasoning

cpojer · 2017-02-27T15:42:02Z

Thanks for the update. Would you mind running the performance test suite before and after your change to know whether the typeof check affects performance at all? Try yarn test-pretty-format-perf.

pedrottimark · 2017-02-27T18:42:09Z

Hi, I’m Mark, and I write sed scripts more slowly than 30 to 40 years ago :)

Totals of prettyFormat() part for two runs each for baseline Jest 19.0.1 and current commit, each run first thing after shutting down and starting fresh. Rebuilt Jest before shutting down.

Short answer is no significant diff in average baseline 42.6025 ± 1.1% and current 42.532 ± 0.8%

baseline has both the min at 42.121 seconds and the max at 43.084 for range of 0.963
current has 42.210 and 42.854 for range of 0.644 enclosed within baseline min and max

cpojer · 2017-02-27T19:41:36Z

I merged another change by @thejameskyle so I'm afraid I need to ask you to rebase one more time. I'm sorry.

pedrottimark · 2017-02-27T19:50:22Z

Oh good, now print and prettyFormat have consistent order: plugin, basic, complex.

Yes, I will rebase carefully, so don’t hold your breath :)

thymikee · 2017-02-27T20:22:44Z

Changes from the future

jamiebuilds · 2017-02-27T20:25:55Z

packages/pretty-format/src/index.js

  return '[' + errorToString.call(val) + ']';
 }

-function printBasicValue(val, printFunctionName, escapeRegex) {
+function printBasicValue(val: any, printFunctionName: boolean, escapeRegex: boolean): StringOrNull {


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

Yes, thank you, in all 8 occurrences?

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

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.

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.

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.

I'm not sure what you're asking 😕

* Add Flow type annotations to pretty-format * Move new test to be among existing tests * modify test of properties and symbols to emphasize correct sorting * exact type for theme and if plugin returns string

github-actions · 2021-05-13T21:07:20Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

Add Flow type annotations to pretty-format

45e77e6

facebook-github-bot added the CLA Signed ✔️ label Feb 22, 2017

Move new test to be among existing tests

6c12642

thymikee reviewed Feb 22, 2017

View reviewed changes

modify test of properties and symbols to emphasize correct sorting

3827a5d

cpojer reviewed Feb 27, 2017

View reviewed changes

cpojer requested changes Feb 27, 2017

View reviewed changes

exact type for theme and if plugin returns string

78dd416

cpojer approved these changes Feb 27, 2017

View reviewed changes

rebase to include changes from 3017

0038d9f

jamiebuilds suggested changes Feb 27, 2017

View reviewed changes

cpojer merged commit 9874b81 into jestjs:master Feb 28, 2017

pedrottimark deleted the flow-pretty-format branch February 28, 2017 16:12

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Flow type annotations to pretty-format #2977

Add Flow type annotations to pretty-format #2977

pedrottimark commented Feb 22, 2017

codecov-io commented Feb 22, 2017 •

edited

Loading

thymikee Feb 22, 2017

pedrottimark Feb 22, 2017

thymikee Feb 22, 2017

pedrottimark Feb 22, 2017 •

edited

Loading

thymikee commented Feb 22, 2017

cpojer Feb 27, 2017

pedrottimark Feb 27, 2017

pedrottimark Feb 27, 2017 •

edited

Loading

cpojer Feb 27, 2017

pedrottimark Feb 27, 2017

cpojer left a comment

cpojer commented Feb 27, 2017

pedrottimark commented Feb 27, 2017

cpojer commented Feb 27, 2017

pedrottimark commented Feb 27, 2017

cpojer commented Feb 27, 2017

pedrottimark commented Feb 27, 2017

thymikee commented Feb 27, 2017

jamiebuilds Feb 27, 2017

pedrottimark Feb 27, 2017

pedrottimark Feb 27, 2017

jamiebuilds Feb 27, 2017 •

edited

Loading

jamiebuilds Feb 27, 2017

pedrottimark Feb 27, 2017

jamiebuilds Feb 27, 2017

github-actions bot commented May 13, 2021

Add Flow type annotations to pretty-format #2977

Add Flow type annotations to pretty-format #2977

Conversation

pedrottimark commented Feb 22, 2017

codecov-io commented Feb 22, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrottimark Feb 22, 2017 • edited Loading

Choose a reason for hiding this comment

thymikee commented Feb 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedrottimark Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

cpojer commented Feb 27, 2017

pedrottimark commented Feb 27, 2017

cpojer commented Feb 27, 2017

pedrottimark commented Feb 27, 2017

cpojer commented Feb 27, 2017

pedrottimark commented Feb 27, 2017

thymikee commented Feb 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiebuilds Feb 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2021

codecov-io commented Feb 22, 2017 •

edited

Loading

pedrottimark Feb 22, 2017 •

edited

Loading

pedrottimark Feb 27, 2017 •

edited

Loading

jamiebuilds Feb 27, 2017 •

edited

Loading