Stringify rewrite #33
Conversation
|
This also fixes #25 |
|
LGTM. Sent from Samsung Galaxy A6+ |
towergame
left a comment
towergame
left a comment
There was a problem hiding this comment.
Looks good, but there are a couple small revisions I'd like :)
src/index.ts
Outdated
| for(let i=0;i<referenceArr.length;i++) { | ||
| if(referenceArr[i][0] === item && depth > referenceArr[i][1]) { | ||
| result = "[unserializable object]"; | ||
| hasNonTrivialReferences = true; |
There was a problem hiding this comment.
Would it not be more informative to return [circular reference] as the result? That way the user will be able to know that it is a circular reference being truncated, rather than some unexplainable object that refuses to be serialized. Otherwise I think we risk the user going on a wild goose chase for a non-issue :P
There was a problem hiding this comment.
I believe non-trivial would be better. This algorithm does not actually properly detect if there are cyclical references, rather, just quits out if it detects that the same object was already referenced a layer higher. I believe this is ok for our purposes, but an optional "thorough serialization" flag could be added later that does element sorting by dependency to log properly at the cost of speed.
There was a problem hiding this comment.
I will rework the algorithm to look for circular references up the current dependency chain, rather than global reference depth counting. This way it can double data in some places but that way they will definitely be circular
Symbols now output with their name attached and extra tests
3 participants
I've done an overhaul on stringification as the previous versions used JSON.stringify, which is unable to handle functions or classes or class instances, as well as
bigints. It also allows for logging circular references, and it will attempt to display as much as possible without repeating data.Additionally a carriage return option has been added to config and the documentation. ( off by default of course :) )
This could close #32 #26 and make #31 obsolete should this PR be merged faster.