-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
[jest-worker]: bigInt serialization in jest-worker #11624
Conversation
Codecov Report
@@ Coverage Diff @@
## master #11624 +/- ##
==========================================
- Coverage 69.01% 69.01% -0.01%
==========================================
Files 312 312
Lines 16335 16339 +4
Branches 4734 4735 +1
==========================================
+ Hits 11274 11276 +2
- Misses 5033 5035 +2
Partials 28 28
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Could you add a test and a changelog entry?
4f7cd03
to
e625b95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems odd to me - how does only serializing work when we don't deserialize? Isn't the BigInt
lost? In which case it'll be reported wrong if passed across a worker
$ node -p 'const JSONbig = require("json-bigint"); JSONbig.parse(JSONbig.stringify({hello: BigInt(42)}));'
[Object: null prototype] { hello: 42 }
I'd like to see a test that sends an object to a worker and receives it back, and verifies they are equal.
This particular module also seems to be failing on Node 10.
Also, at some point we need to add support for circular objects (#10577) for which this module won't work. We can tackle that later of course, just showing that we need to properly serialize/deserialize 🙂
@@ -45,6 +46,10 @@ function getExposedMethods( | |||
return exposedMethods; | |||
} | |||
|
|||
export function serializerWithBigInt(data: unknown): unknown { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be in index
to avoid circular dependencies. Please stick it into a utils
, serializers
or something
This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days. |
This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one. |
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. |
Summary
Resolves #11617
Adds serialization support for BigInt Type - when passing messages through
jest-worker
processes (parallel runs not usingworker-threads
)Test plan
BigIntSerialization
for messages sent through jest-workerUpdate