Don't wrap multiline strings with double quotes #4183

thymikee · 2017-08-02T17:18:51Z

Summary

This is a breaking change and needs updating snapshot version to v2 (v1 was introduced in Jest 19)

This PR treats multiline strings specially and wrap them in back-ticks instead of double quotes.
pretty-format strips extra back-ticks just before returning the value. This is because we wrap whole expression in such back-ticks when writing a snapshot.

Fixes #4179.

Test plan

Updated the snapshots accordingly. This way we can discuss easier on whether this change is worth breaking next Jest version.

cc @cpojer @aaronabramov @vjeux @rogeliog

thymikee · 2017-08-02T17:23:20Z

integration_tests/__tests__/__snapshots__/custom_reporters.test.js.snap

    }
-}"


💩, it's so much more readable

aaronabramov · 2017-08-02T17:51:31Z

can we not merge it for a few days? :) a want to land my diff to update jest in www first (i already have a diff open for it), with this PR my diff can explode to many hundred thousand lines changed :D

thymikee · 2017-08-02T17:56:54Z

Sure, I'll need to figure out what's wrong with these color tags on CI :<

pedrottimark · 2017-08-02T20:06:48Z

The example of object property value which is a multiline string looks 👍 to me.

Could the snapshot module take responsibility to strip surrounding backticks when needed?

Stripping them in pretty-format might be confusing outside of snapshots. For example:

prettyFormat('one line') === '"one line"' explicitly represents a string value
prettyFormat('two\nlines') === 'two\nlines' has neither quotes nor backticks?

thymikee · 2017-08-02T20:08:44Z

@pedrottimark you're right about stripping backticks in snapshots. I'll iterate over tomorrow. Thanks for your input!

aaronabramov · 2017-08-03T00:27:10Z

just a heads up, i landed my www diff! so it's good to go

thymikee · 2017-08-03T05:41:22Z

congrats! 🎉

cpojer · 2017-08-03T09:38:52Z

Let's give it another day, CI is failing also fyi.

thymikee · 2017-08-04T07:22:30Z

packages/pretty-format/src/index.js

@@ -106,7 +106,9 @@ function printBasicValue(
    return printNumber(val);
  }
  if (typeOf === 'string') {
-    return '"' + val.replace(/"|\\/g, '\\$&') + '"';
+    return /\n/.test(val)
+      ? '`' + val.replace(/\\/g, '\\$&') + '`'


Turned out that not escaping backslashes (this code was previously ? '`' + val + '`') introduced differences in color output of child processes. ¯_(ツ)_/¯
cc @aaronabramov

thymikee · 2017-08-04T09:06:40Z

Ok, looks like the pretty-format and jest-snapshot are not linked properly on Windows. See #4135

codecov-io · 2017-08-07T21:26:00Z

Codecov Report

❗ No coverage uploaded for pull request base (master@f7b25b6). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             master   #4183   +/-   ##
========================================
  Coverage          ?   60.2%           
========================================
  Files             ?     191           
  Lines             ?    6768           
  Branches          ?       6           
========================================
  Hits              ?    4075           
  Misses            ?    2690           
  Partials          ?       3

Impacted Files	Coverage Δ
packages/pretty-format/src/index.js	`97.7% <100%> (ø)`
packages/jest-snapshot/src/utils.js	`100% <100%> (ø)`

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 f7b25b6...7dc5693. Read the comment docs.

thymikee · 2017-08-20T17:09:49Z

BTW, we can postpone this PR to anytime later, it's not a blocker for me anymore. Nevertheless, I'd still like to land it one day.

cpojer · 2017-08-22T23:27:29Z

Alright, I think I really want us to adopt this and have a new version of the snapshot format for 21, however I think it would be great if we could do a simpler rollout of the new snapshot format for 21:

How about for the current version of snapshots, we support printing the quotes, but if a snapshot changes, any update to that individual file will update to the new snapshot version? That way, Jest 21 can support the current format, and for all new and updated snapshots it will support the new format. I really think that we should prevent churning users when making an upgrade, and it will also make it much easier to upgrade Jest on large codebases. @thymikee what do you think? Could you make this change?

thymikee · 2017-08-23T07:51:37Z

Hopefully I'll find some time for this by the end of the week(end)

pedrottimark · 2017-08-28T22:08:40Z

Really looking forward to this!

Does first branch of ternary need to escape backticks as other branch escapes double quotes:

  if (typeOf === 'string') {
    return /\r\n?|\n/gm.test(val)
      ? '`' + val.replace(/\\/g, '\\$&') + '`'
      : '"' + val.replace(/"|\\/g, '\\$&') + '"';
  }

thymikee · 2017-08-29T04:40:19Z

I remember wrapping it because I needed a way to determine multi-line string while unescaping in jest-snapshot.

pedrottimark · 2017-08-29T14:35:44Z

Indeed, I forgot that jest-snapshot already escapes backticks in https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/utils.js#L147-L149

Thinking about it again, hopefully with a clearer mind, I think what I should have meant to ask is when pretty-format is used for purpose other than to serialize a snapshot (for example, by jest-diff when an assertion fails) doesn’t it need to escape backticks that occur in multi line strings just as it escapes double quotes that occur in single line strings? If the answer is yes, then it becomes necessary for jest-snapshot not to double-escape them.

pedrottimark · 2017-09-20T15:42:26Z

@thymikee when you come back to this pull request, I noticed while studying trailing newlines in jest-diff that for correct comparison of multiline strings, it might become necessary to trim exactly the one leading and trailing newline added when snapshot was written in:

https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/utils.js#L132-L133

instead of unrestricted String.trim method in:

https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/index.js#L89-L90

cpojer · 2017-12-15T11:06:07Z

I still want this but I'm gonna close it for now because nobody is actively working on it. Let's create a new PR if we make this change.

github-actions · 2021-05-13T01:32:01Z

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.

facebook-github-bot added the CLA Signed ✔️ label Aug 2, 2017

thymikee commented Aug 2, 2017

View reviewed changes

thymikee force-pushed the multiline-strings branch from aa072a7 to 785c732 Compare August 3, 2017 12:18

thymikee commented Aug 4, 2017

View reviewed changes

thymikee force-pushed the multiline-strings branch from e1ccc4a to 5d220a8 Compare August 4, 2017 09:01

thymikee added 6 commits August 7, 2017 22:17

Don't wrap multiline strings with double quotes

5e11e72

Move removeWrappingBackticks to snapshots

06ef7c6

Escape '\' in multiline strings like in single ones

7cfa92b

Test for windows specific newlines

6386f1f

Debug Windows

abcc6da

Remove windows logging

4d4f0d9

thymikee force-pushed the multiline-strings branch from 5d220a8 to 4d4f0d9 Compare August 7, 2017 20:20

Fix flaky jest.config.js test

7dc5693

This was referenced Aug 9, 2017

toMatchSnapshot(filepath) #4231

Closed

More jest integration jest-community/snapshot-diff#10

Closed

pedrottimark mentioned this pull request Aug 22, 2017

Ignore indentation of data structures in jest-diff #3429

Merged

cpojer mentioned this pull request Aug 22, 2017

Add displayName to MPR #4327

Merged

1 task

cpojer mentioned this pull request Aug 23, 2017

Jest 21 Master Task #4332

Closed

6 tasks

pedrottimark mentioned this pull request Sep 20, 2017

Display outer element and trailing newline consistently in jest-diff #4520

Merged

cpojer closed this Dec 15, 2017

SimenB mentioned this pull request Jun 3, 2019

chore: use jest-serializer-raw for react-fresh snapshots facebook/react#15806

Merged

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

Don't wrap multiline strings with double quotes #4183

Don't wrap multiline strings with double quotes #4183

thymikee commented Aug 2, 2017

thymikee Aug 2, 2017 •

edited

Loading

aaronabramov commented Aug 2, 2017

thymikee commented Aug 2, 2017

pedrottimark commented Aug 2, 2017

thymikee commented Aug 2, 2017

aaronabramov commented Aug 3, 2017

thymikee commented Aug 3, 2017

cpojer commented Aug 3, 2017

thymikee Aug 4, 2017 •

edited

Loading

thymikee commented Aug 4, 2017

codecov-io commented Aug 7, 2017

thymikee commented Aug 20, 2017

cpojer commented Aug 22, 2017

thymikee commented Aug 23, 2017

pedrottimark commented Aug 28, 2017

thymikee commented Aug 29, 2017

pedrottimark commented Aug 29, 2017

pedrottimark commented Sep 20, 2017

cpojer commented Dec 15, 2017

github-actions bot commented May 13, 2021

Don't wrap multiline strings with double quotes #4183

Don't wrap multiline strings with double quotes #4183

Conversation

thymikee commented Aug 2, 2017

thymikee Aug 2, 2017 • edited Loading

Choose a reason for hiding this comment

aaronabramov commented Aug 2, 2017

thymikee commented Aug 2, 2017

pedrottimark commented Aug 2, 2017

thymikee commented Aug 2, 2017

aaronabramov commented Aug 3, 2017

thymikee commented Aug 3, 2017

cpojer commented Aug 3, 2017

thymikee Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

thymikee commented Aug 4, 2017

codecov-io commented Aug 7, 2017

Codecov Report

thymikee commented Aug 20, 2017

cpojer commented Aug 22, 2017

thymikee commented Aug 23, 2017

pedrottimark commented Aug 28, 2017

thymikee commented Aug 29, 2017

pedrottimark commented Aug 29, 2017

pedrottimark commented Sep 20, 2017

cpojer commented Dec 15, 2017

github-actions bot commented May 13, 2021

thymikee Aug 2, 2017 •

edited

Loading

thymikee Aug 4, 2017 •

edited

Loading