🐛 [RUMF-1410] Allow serialization of objects with cyclic references #1783
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
|
yannickadam
force-pushed
the
yannick/rumf-1410-serialize-cyclic-object
branch
2 times, most recently
from
2000bb1 to
14cd826
Compare
Codecov Report
@@ Coverage Diff @@
## main #1783 +/- ##
==========================================
+ Coverage 90.92% 91.10% +0.17%
==========================================
Files 129 129
Lines 5038 5037 -1
Branches 1133 1132 -1
==========================================
+ Hits 4581 4589 +8
+ Misses 457 448 -9
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
yannickadam
force-pushed
the
yannick/rumf-1410-serialize-cyclic-object
branch
from
14cd826 to
c56f53f
Compare
liywjl
approved these changes
Oct 21, 2022
bcaudan
approved these changes
Oct 21, 2022
packages/core/src/tools/utils.ts
Outdated
Comment on lines
172
to
177
| const isValidObject = (v: any): v is object => typeof v === 'object' && v !== null | ||
| const getCyclicReplacer = <T>() => { | ||
| // Using a weakmap instead of a weakset to support IE11 | ||
| const visited = new WeakMap<object, boolean>() | ||
| return (_k: string, v: T) => { | ||
| if (isValidObject(v)) { |
There was a problem hiding this comment.
🥜 nitpick: using a meaningful word instead of a letter could be easier to follow, something like candidate or value instead of v.
|
Looks great! |
amortemousque
approved these changes
Oct 21, 2022
liywjl
approved these changes
Oct 25, 2022
bcaudan
approved these changes
Oct 25, 2022
packages/core/src/tools/utils.ts
Outdated
| // subclasses. | ||
| const restoreObjectPrototypeToJson = detachToJsonMethod(Object.prototype) | ||
| const restoreArrayPrototypeToJson = detachToJsonMethod(Array.prototype) | ||
| const restoreValuePrototypeToJson = detachToJsonMethod(Object.getPrototypeOf(value)) | ||
| const restoreValueToJson = detachToJsonMethod(value) | ||
|
|
||
| // Line below can be removed when migrating to TS4.8+ | ||
| const isValidObject = (v: any): v is object => typeof v === 'object' && v !== null |
There was a problem hiding this comment.
💬 suggestion: use getType(destination) === 'object' instead
packages/core/src/tools/utils.ts
Outdated
| // subclasses. | ||
| const restoreObjectPrototypeToJson = detachToJsonMethod(Object.prototype) | ||
| const restoreArrayPrototypeToJson = detachToJsonMethod(Array.prototype) | ||
| const restoreValuePrototypeToJson = detachToJsonMethod(Object.getPrototypeOf(value)) | ||
| const restoreValueToJson = detachToJsonMethod(value) | ||
|
|
||
| // Line below can be removed when migrating to TS4.8+ | ||
| const isValidObject = (v: any): v is object => typeof v === 'object' && v !== null | ||
| const getCyclicReplacer = <T>() => { |
There was a problem hiding this comment.
🥜 nitpick: the type parameter T isn't useful here, and could be replaced by unknown in the argument list
🥜 nitpick: FMU you create a closure just to keep the circularReferenceChecker variable private. IMO this is not worth it and the following would be a bit easier to read:
const circularReferenceChecker = createCircularReferenceChecker()
const cyclicReplacer = (_key: string, value: unknown) => {
if (isValidObject(value) && circularReferenceChecker.hasAlreadyBeenSeen(value)) {
return '<warning: cyclic reference not serialized>'
}
return value
}| } | ||
| const array: any[] = [] | ||
| // Using a weakmap instead of a weakset to support IE11 | ||
| const map: WeakMap<any, boolean> = new WeakMap() |
There was a problem hiding this comment.
👏 praise: great, love it :)
|
/to-staging |
|
🚂 MergeFlow Merge branch is merge-1783-to-staging-44 |
yannickadam
force-pushed
the
yannick/rumf-1410-serialize-cyclic-object
branch
from
5b254b4 to
146da85
Compare
BenoitZugmeyer
approved these changes
Oct 26, 2022
yannickadam
added a commit
that referenced
this pull request
Nov 17, 2022
4 tasks
yannickadam
added a commit
that referenced
this pull request
Nov 17, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Fix for #807
Changes
Added a replacer within the jsonStringify function that removes cyclic references while serializing.
Testing
I have gone over the contributing documentation.