Add jest-serializer module #5609

mjesun · 2018-02-19T11:36:01Z

This PR adds a new jest-serializer module, which combines V8 and JSON serializers, with APIs to serialize/deserialize in memory and/or from disk, either synchronously or asynchronously, which can later be used in jest-haste-map (as shown on the PR), or in jest-worker, to communicate between parent and child processes.

Tests were added to it covering all APIs with a variety of objects.

Resolves #5575

thymikee · 2018-02-19T11:40:49Z

packages/jest-serializer/README.md

+pipes.
+
+```javascript
+import serializer from 'jest-serializer';


Destructuring maybe?

import {serialize, deserialize} from 'jest-serializer';

thymikee · 2018-02-19T11:42:45Z

packages/jest-serializer/src/__tests__/index.test.js

+ * Copyright (c) 2018-present, Facebook, Inc. All rights reserved.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.


Some Flow maybe?

Ah, pardon this is a test (so it's not necessary to flow it, but I like to mix these)

jukben · 2018-02-19T11:42:58Z

packages/jest-serializer/package.json

+  "license": "MIT",
+  "main": "build/index.js",
+  "dependencies": {
+    "merge-stream": "^1.0.1"


Where is used this dependency?

Nowhere 😳I just copy-pasted the jest-worker one and forgot to remove.

cpojer · 2018-02-19T11:50:03Z

I like this but I have two high-level comments:

Do we really need all the fs APIs in there? If yes, do we really need the async versions or can we just expose the sync ones? Then users can compose the async ones themselves and the sync ones are available. Mainly asking because the API surface would be smaller. Even better would be to only expose serialize/deserialize but I can see how the helpers are useful.
This now creates an interface that has different behaviors based on the node version, without warnings. I don't think we can merge it like this. For example, on node 8, serializing a Map will work whereas on node 6 it won't work. I'd like the JSON implementation to keep working or at least throwing an error if it can't work on that version of node. I think using reviver/serializer is fine but see https://github.com/cognitect/transit-js for other types of serializers.

mjesun · 2018-02-19T12:23:10Z

I would prefer to keep both fs versions, because they are mutually exclusive (you cannot make the async work as sync and vice versa), but I'm fine removing one of them.
You are right regarding V8 vs JSON serialization. I will add a reviver that can handle: undefined, NaN, Infinity, -Infinity, Date, RegExp, Set, Map and Buffer. I think we'll be in a good position with these types.

cpojer · 2018-02-19T12:37:02Z

Let's remove the async ones until we need them. What I meant you can manually call fs.writeFile(file, serialize(data), callback) and that's fine.

cpojer · 2018-02-19T12:37:48Z

packages/jest-serializer/src/index.js

+    : JSON.parse(fs.readFileSync(file, 'utf8'));
+}
+
+export function writeFileSync(file: Path, content: any) {


can you call this filePath?

SimenB · 2018-02-19T14:05:24Z

packages/jest-serializer/src/index.js

+
+// Asynchronous filesystem functions.
+
+export function readFile(file: Path, callback: IOCallback) {


if we expose an async API, I'd prefer it to be promise instead of callback based

I just killed the async one 😜. But you are right; it should have been Promise based. I just followed the standard Node JS format.

The reason for killing it is because we don't really need it anywhere: neither in jest-haste-map, nor in jest-worker (coming soon), nor in Metro, where we will use this module as well.

codecov-io · 2018-02-19T16:04:29Z

Codecov Report

Merging #5609 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #5609      +/-   ##
==========================================
+ Coverage   60.63%   60.64%   +<.01%     
==========================================
  Files         213      214       +1     
  Lines        7311     7310       -1     
  Branches        3        4       +1     
==========================================
  Hits         4433     4433              
+ Misses       2877     2876       -1     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-cli/src/search_source.js	`45.07% <ø> (ø)`	⬆️
packages/jest-haste-map/src/index.js	`97.16% <100%> (+0.85%)`	⬆️
packages/jest-serializer/src/index.js	`92.85% <100%> (ø)`
.../src/legacy_code_todo_rewrite/jest_adapter_init.js	`0% <0%> (ø)`	⬆️
packages/jest-config/src/normalize.js	`93.4% <0%> (ø)`	⬆️

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 f5879c0...c715d6f. Read the comment docs.

thymikee

LGTM

thymikee · 2018-02-19T16:48:01Z

packages/jest-serializer/src/__tests__/index.test.js

+
+'use strict';
+
+import prettyFormat from 'pretty-format';


pretty-format should go to devDependencies now

Right! good catch!

Oh, actually no 😅. There's no way you can develop jest without having pretty-format (yarn complained when trying to add it, and it ended up adding it to the root package.json instead of the child one).

Ahh, I thought about this because it's going to be a standalone package, but then again, it's still inside a workspace monorepo. 👍

thymikee · 2018-02-19T17:00:43Z

packages/jest-serializer/src/__tests__/index.test.js

+
+          expect(buf).toBeInstanceOf(Buffer);
+
+          expect(prettyFormat(serializer.deserialize(buf))).toEqual(


Nice workaround on different different contexts ^^

thymikee · 2018-02-19T17:07:20Z

packages/jest-serializer/src/index.js

+      return {[JS_TYPE]: 'm', [JS_VALUE]: Array.from(value)};
+
+    case Buffer:
+      return {[JS_TYPE]: 'b', [JS_VALUE]: value.toString('latin1')};


Any particular reason behind latin1 encoding?

It's the most optimal way of converting a buffer into a string. Other candidates would be an array (way too long to decode), a UTF-8 string (but not all sequences you might find are valid sequences), or a hex one (which is going to longer or at most as equal as a latin1 string) 🙂

github-actions · 2021-05-12T20:06:56Z

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 jest-serializer module

bca7ccf

mjesun requested a review from cpojer February 19, 2018 11:36

facebook-github-bot added the CLA Signed ✔️ label Feb 19, 2018

thymikee reviewed Feb 19, 2018

View reviewed changes

jukben reviewed Feb 19, 2018

View reviewed changes

jukben mentioned this pull request Feb 19, 2018

Create a jest-serialize package that uses either v8 serialize or JSON #5575

Closed

Address feedback: "package.json" + "README.md"

ea468e5

cpojer reviewed Feb 19, 2018

View reviewed changes

SimenB reviewed Feb 19, 2018

View reviewed changes

mjesun mentioned this pull request Feb 19, 2018

Make jest-worker use jest-serializer #5613

Closed

Address feedback: allow JSON stringification to handle native types

c715d6f

thymikee approved these changes Feb 19, 2018

View reviewed changes

mjesun merged commit 3a2854a into jestjs:master Feb 19, 2018

mjesun deleted the jest-serializer branch February 19, 2018 18:07

SimenB added Needs Changelog and removed Needs Changelog labels Feb 19, 2018

This was referenced Mar 5, 2018

Update jest-environment-jsdom in / from 22.2.0 to 22.4.1 opencomponents/base-templates#271

Closed

Update jest in / from 22.2.1 to 22.4.2 opencomponents/base-templates#272

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add jest-serializer module #5609

Add jest-serializer module #5609

mjesun commented Feb 19, 2018 •

edited by thymikee

Loading

thymikee Feb 19, 2018

thymikee Feb 19, 2018

thymikee Feb 19, 2018

jukben Feb 19, 2018

mjesun Feb 19, 2018

cpojer commented Feb 19, 2018

mjesun commented Feb 19, 2018

cpojer commented Feb 19, 2018

cpojer Feb 19, 2018

SimenB Feb 19, 2018 •

edited

Loading

mjesun Feb 19, 2018

codecov-io commented Feb 19, 2018

thymikee left a comment

thymikee Feb 19, 2018

mjesun Feb 19, 2018

mjesun Feb 19, 2018

thymikee Feb 19, 2018

thymikee Feb 19, 2018

mjesun Feb 19, 2018

thymikee Feb 19, 2018

mjesun Feb 19, 2018

github-actions bot commented May 12, 2021


		// Asynchronous filesystem functions.

		export function readFile(file: Path, callback: IOCallback) {


		expect(buf).toBeInstanceOf(Buffer);

		expect(prettyFormat(serializer.deserialize(buf))).toEqual(

Add jest-serializer module #5609

Add jest-serializer module #5609

Conversation

mjesun commented Feb 19, 2018 • edited by thymikee 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

Choose a reason for hiding this comment

cpojer commented Feb 19, 2018

mjesun commented Feb 19, 2018

cpojer commented Feb 19, 2018

Choose a reason for hiding this comment

SimenB Feb 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 19, 2018

Codecov Report

thymikee left a comment

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

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

Choose a reason for hiding this comment

github-actions bot commented May 12, 2021

mjesun commented Feb 19, 2018 •

edited by thymikee

Loading

SimenB Feb 19, 2018 •

edited

Loading