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.

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

Avoid unnecessary function declarations and call in pretty-format #3962

Merged
merged 3 commits into from
Jul 4, 2017

Conversation

pedrottimark
Copy link
Contributor

Summary

Problem:

  • printPlugin “hoists” the boundPrint and boundIndent declarations and therefore initializes the functions whether or not a plugin matches the value.
  • prettyFormat calls createIndent if any plugins exist instead of if some plugin matches the value. Because plugins always exist in Jest, the conditional logic to avoid creating indentation for basic values doesn’t save any time.

Solution:

  • Factor out a findPlugin function.
  • Factor out if statement to call printPlugin only if a plugin matches the value.
  • Call createIndent for arg in printPlugin and printComplexValue function calls.

Your review is especially wanted about the decision to throw an error if a plugin returns a non-string value. Am happy to revert that change if it’s a step in a wrong direction. Current logic quietly falls through to print the value as either basic or complex.

Interpretation of the following performance measurements:

  • Big improvement for massive object with no plugins outside Jest.
  • Almost as big improvement with default plugins (not called) inside Jest.
  • Only slight change for small elements with default plugins (called) inside Jest.
scenario plugins proposed baseline ratio
geo.json 0 30693309 72540558 0.423
geo.json 10 70169013 127109022 0.552
React elements 10 1283359 1287893 0.996

P.S. The calls to printPlugin are nice example of not ignoring indentation in a diff, heh :)

Test plan

Jest

@codecov-io
Copy link

codecov-io commented Jul 3, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3962      +/-   ##
==========================================
- Coverage   59.86%   59.84%   -0.03%     
==========================================
  Files         196      196              
  Lines        6763     6758       -5     
  Branches        6        6              
==========================================
- Hits         4049     4044       -5     
  Misses       2711     2711              
  Partials        3        3
Impacted Files Coverage Δ
packages/pretty-format/src/index.js 98% <100%> (-0.05%) ⬇️

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 b079d2a...77bae77. Read the comment docs.

@cpojer
Copy link
Member

cpojer commented Jul 4, 2017

This is great, thanks so much for optimizing the code here. How did you track this down, did you do some profiling?

As for throwing an error: while technically a breaking change, the type of the print function of a Plugin is defined to return a string. I think your added constraint in the code makes sense, given that we don't have control over the behavior of user plugins. While it may be beneficial to return another value that will then be printed by prettyFormat, you have the print function available from within plugins so we aren't limiting the functionality of plugins.

Thanks for coming up with this PR!

@pedrottimark
Copy link
Contributor Author

You’re welcome. Something didn’t feel right in the function when I thought about a code change to support alternative API discussed in #3518 (comment). You would laugh to see a scan of my Pooh Bear brain puzzling it out. The performance improvements are a side effect of making the package more correct and clear where it has been hard for me understand, so a wider range of people might contribute successfully in the future to keep up with ECMAScript and application-specific data types.

When Jest turns 21 people can reasonably expect it to break some things, heh?

@pedrottimark pedrottimark deleted the avoid-declarations-call branch July 5, 2017 13:43
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
…stjs#3962)

* Avoid unnecessary function declarations and call in pretty-format

* Add missing newline and quotes

* Move throw into printPlugin
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants