Add support for Immutable.Record in pretty-format #3678
Conversation
renchap
force-pushed
the
pretty-format-immutable-record
branch
from
6f681c6 to
e782cbc
Compare
Codecov Report
@@ Coverage Diff @@
## master #3678 +/- ##
==========================================
+ Coverage 62.57% 62.63% +0.05%
==========================================
Files 181 182 +1
Lines 6694 6704 +10
Branches 6 6
==========================================
+ Hits 4189 4199 +10
Misses 2502 2502
Partials 3 3
Continue to review full report at Codecov.
|
| immutableArray.push( | ||
| indent(addKey(isMap, key) + print(item, print, indent, opts, colors)), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Can you extract a function and reuse it? Something like:
const pushToImmutableArray = (item: any, key: string) => {
immutableArray.push(
indent(addKey(isMap, key) + print(item, print, indent, opts, colors)),
)
}
|
|
||
| if (Array.isArray(val._keys)) { | ||
| // if we have a record, we can not iterate on it directly | ||
| val._keys.forEach(key => pushToImmutableArray(val.get(key), key)); |
There was a problem hiding this comment.
We are looking at private fields here and with ._name above. I'm worried about changes in Immutable that will break this and therefore break snapshots.
cc @leebyron do you have any input for us on what we should do here? Do you think this is fine for pretty-printing?
There was a problem hiding this comment.
I think this is fine as long as someone is prepared to update this code if Immutable.js implementation details ever change, which is entirely possible. Though I think there are options for improvements
For ._name, you might want to sniff for Immutable v4.0 which offers Record.getDescriptiveName(record), otherwise use this fallback implementation, however note that your implementation differs from Immutable.js's.
I think ._keys is less safe to use. Since you just want to iterate over these, you can probably cast to a lazy Seq first. So lines 48-53 here can be replaced with:
val.toSeq().forEach(pushToImmutableArray)
|
I merged this even though it currently accesses some private fields. We already do duck-typing for the immutable types and that could change any time, the private fields aren't any different. However, it would be great if immutable had an official introspection API for debugging. If it does, or if it gets added, we should switch to that asap. |
|
There is an introspective API, for example:
I used private fields in my patch to be consistent with the original implementation. From my quick check, the only thing that is not accessible is the list of attributs in a |
|
yep, you are right. We are trying not to require immutable at all. If the introspective API was available through objects from immutable so we can avoid requiring immutable (or any one version of immutable) directly, then we can use them. |
* Add support for Immutable.Record in pretty-format Fixes jestjs#3677 * Extract pushToImmutableArray function
|
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
Immutable.Recordare not handled bypretty-print, but other Immutable.js structures are.Recordare often used in Redux states for example, and Redux reducers tested using snapshots. This will allow the snapshots to be more precise, referencing properImmutable.Recordand not plain JS objects.Test plan
I added tests for the various usecases I had in mind.
This fixes #3677
@thymikee can you review it? I tried to not add anything Record-specific in
lib/printImmutable.jsand rely onval._keysandval._name.val._nameis only used forRecord, butImmutable.Seq(which is not supported bypretty-print) also has._keys. Maybe I should check ifval.forEachis defined and fallback toval._keys? Or should I specificaly check if a record is passed?