Replace print with serialize in Immutable plugins by pedrottimark · Pull Request #4189 · jestjs/jest

pedrottimark · 2017-08-03T17:40:33Z

Summary

@cpenarrieta your feedback is welcome :) for more info, see #4114 and #4184

Breaking change in Map, OrderedMap, and Record to print the key:

because implicit conversion to string throws an error if key is symbol
because implicit conversion to string is [object Object] if key is object
to distinguish primitive values false (and so on) from "false" as string

Breaking changes for edge cases:

support maxDepth option
support indentation of immutable collection as child of non-immutable (the closing punctuation was incorrect in the same way as for closing tag in array of React elements)

Question: Although I see a value in the line break between brackets/braces for non-minified empty Immutable collections (see #4121) now that lib/immutable.js calls the core helper functions, and the next version of Jest will have breaking changes for snapshots, it is a good time to review whether that difference is worth the extra code logic.

Test plan

Updated: string keys in double quote marks

Added:

supports non-string keys
indentation of heterogeneous collections
indent option
maxDepth option

pedrottimark · 2017-08-03T20:20:50Z

Updated 3 snapshot tests in packages/jest-matchers/src/__tests__/spy_matchers.test.js

To replace copy-paste-reuse, wrote my first iterator with help from Understanding ECMAScript 6

pedrottimark · 2017-08-03T21:47:13Z

AppVeyor build failed: spy_matchers.test.js 3 received values don’t show the changes to plugin

Related to #4135 ?

pedrottimark · 2017-08-04T13:10:50Z

Two questions about possible additional (or should I say, subtractional) simplifications:

Do test expressions require !! explicit coercion to boolean?
Could we merge OrderedMap into Map and OrderedSet into Set plugin as follows:

export const test = (val: any) => val && val[IS_MAP];
export const serialize = (
  val: any,
  config: Config,
  indentation: string,
  depth: number,
  refs: Refs,
  printer: Printer,
) =>
  printImmutableEntries(
    val,
    config,
    indentation,
    depth,
    refs,
    printer,
    val[IS_ORDERED] ? 'OrderedMap' : 'Map',
  );

export const test = (val: any) => val && val[IS_SET];
export const serialize = (
  val: any,
  config: Config,
  indentation: string,
  depth: number,
  refs: Refs,
  printer: Printer,
) =>
  printImmutableValues(
    val,
    config,
    indentation,
    depth,
    refs,
    printer,
    val[IS_ORDERED] ? 'OrderedSet' : 'Set',
  );

pedrottimark · 2017-08-05T18:32:25Z

And then there was one!

I worried that it’s a wrecking change, but [ReactTestComponent, ReactElement, HTMLElement].concat(Immutable) works whether Immutable is one plugin or an array of plugins

thymikee

I like the change (wanted to do it myself at some point :D)

thymikee · 2017-08-05T20:38:22Z

packages/pretty-format/src/plugins/immutable.js

+
+const IS_ITERABLE_SENTINEL = '@@__IMMUTABLE_ITERABLE__@@';
+const IS_RECORD_SENTINEL = '@@__IMMUTABLE_RECORD__@@'; // v4 or later
+


I think we can remove these newlines between consts and between imports

thymikee · 2017-08-05T20:43:16Z

packages/pretty-format/src/plugins/immutable.js

+  printer: Printer,
+): string => {
+  if (val[IS_SEQ_SENTINEL]) {
+    return '[' + getName('Seq') + ']';


Do we plan to support Immutable.Seq?

Because Record doesn’t have a sentinel in immutable v3, serialize needs to account for all types that do. The unified test matches Seq too, which seems easiest to understand. Do you mean support it better?

Yup, was thinking about adding it later, if we need it. But not as a part of this PR, let's keep it tight

thymikee · 2017-08-05T20:47:15Z

packages/jest-matchers/src/__tests__/__snapshots__/spy_matchers.test.js.snap


 Expected mock function to not have been last called with:
-  <green>[Immutable.Map {a: {\\"b\\": \\"c\\"}}, Immutable.Map {a: {\\"b\\": \\"c\\"}}]</>"
+  <green>[Immutable.Map {\\"a\\": {\\"b\\": \\"c\\"}}, Immutable.Map {\\"a\\": {\\"b\\": \\"c\\"}}]</>"


This is breaking, but we're more consistent 👍

thymikee · 2017-08-05T20:52:03Z

packages/pretty-format/src/__tests__/immutable.test.js

+      '  Immutable.Map {',
+      '    "key": "value",',
+      '  }: "immutable map",',
+      '}',


I think at some point we should switch such tests to template literals plus dedent

pedrottimark · 2017-08-05T21:11:29Z

Renamed getName as getImmutableName
Moved Seq after all other types except Record (to prioritize Map and List types)
Deleted empty lines as requested

thymikee

LGTM

pedrottimark · 2017-08-05T21:14:33Z

Thank you for investing the time to review. I don’t take it for granted.

thymikee · 2017-08-05T21:16:49Z

Still need to wait until #4135 is resolved (same thing happening in my snapshot breaking PR)

thymikee · 2017-08-05T21:32:31Z

packages/pretty-format/src/plugins/immutable.js

+const getImmutableName = name => 'Immutable.' + name;
+const SPACE = ' ';
+
+const printImmutableEntries = (


There's a lot of duplication in printImmutableEntries and printImmutableValues, but I think that extracting common logic would make it harder to understand what's going on. Not sure but I have no strong feelings about this though. Keep it as you like

* Replace print with serialize in Immutable plugins * Delete multiline string key to avoid conflict with 4183 * Update 3 snapshot tests * Replace printRecordProperties with getRecordIterator * Export 3 print functions from renamed immutable.js file * Move comment * Roll up immutable plugins * Replace getName with getImmutableName * Move Seq after all other types except Record * Delete newlines between adjacent import and sentinel lines

github-actions · 2021-05-13T09:07:08Z

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.

pedrottimark added 2 commits August 3, 2017 13:28

Replace print with serialize in Immutable plugins

5b5d682

Delete multiline string key to avoid conflict with 4183

ab6b3d3

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

pedrottimark added 2 commits August 3, 2017 16:18

Update 3 snapshot tests

257dc40

Replace printRecordProperties with getRecordIterator

02260c3

pedrottimark added 2 commits August 4, 2017 08:38

Export 3 print functions from renamed immutable.js file

83ea633

Move comment

88ec7e8

Roll up immutable plugins

076e952

Replace getName with getImmutableName

4affd2c

thymikee reviewed Aug 5, 2017

View reviewed changes

pedrottimark added 2 commits August 5, 2017 17:05

Move Seq after all other types except Record

248bc02

Delete newlines between adjacent import and sentinel lines

106e8ec

thymikee approved these changes Aug 5, 2017

View reviewed changes

thymikee reviewed Aug 5, 2017

View reviewed changes

cpojer merged commit ee928fd into jestjs:master Aug 6, 2017

pedrottimark deleted the immutable-serialize branch August 6, 2017 17:48

This was referenced Aug 7, 2017

Replace concat(Immutable) with Immutable as item of plugins array #4207

Merged

Improve Seq and remove newline from non-min empty in Immutable plugin #4241

Merged

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


		const IS_ITERABLE_SENTINEL = '@@__IMMUTABLE_ITERABLE__@@';
		const IS_RECORD_SENTINEL = '@@__IMMUTABLE_RECORD__@@'; // v4 or later

Uh oh!

Conversation

pedrottimark commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrottimark commented Aug 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrottimark commented Aug 3, 2017

Uh oh!

pedrottimark commented Aug 4, 2017

Uh oh!

pedrottimark commented Aug 5, 2017

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

thymikee Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

thymikee Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

pedrottimark Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

thymikee Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

thymikee Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

thymikee Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

pedrottimark commented Aug 5, 2017

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

pedrottimark commented Aug 5, 2017

Uh oh!

thymikee commented Aug 5, 2017

Uh oh!

thymikee Aug 5, 2017

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 13, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pedrottimark commented Aug 3, 2017 •

edited

Loading

pedrottimark commented Aug 3, 2017 •

edited

Loading