Skip to content

Commit

Permalink
Enable maxDepth option for tracking depth
Browse files Browse the repository at this point in the history
  • Loading branch information
zalmoxisus authored and blakeembrey committed Aug 8, 2016
1 parent 4da13e4 commit 8401708
Show file tree
Hide file tree
Showing 3 changed files with 54 additions and 14 deletions.
8 changes: 7 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,15 @@ define(function (require, exports, module) {
## Usage

```javascript
javascriptStringify(value[, replacer [, space]])
javascriptStringify(value[, replacer [, space [, options]]])
```

The API is similar to `JSON.stringify`. However, any value returned by the replacer will be used literally. For this reason, the replacer is passed three arguments - `value`, `indentation` and `stringify`. If you need to continue the stringification process inside your replacer, you can call `stringify` with the updated value.

The `options` object allows some additional configuration:

* **maxDepth** The maximum depth to stringify to

### Examples

```javascript
Expand All @@ -52,6 +56,8 @@ javascriptStringify('foo'); // "'foo'"
javascriptStringify({ x: 5, y: 6}); // "{x:5,y:6}"
javascriptStringify([1, 2, 3, 'string']); // "[1,2,3,'string']"

javascriptStringify({ a: { b: { c: 1 } } }, null, null, { maxDepth: 2 }); // "{a:{b:{}}}"

/**
* Invalid key names are automatically stringified.
*/
Expand Down
27 changes: 20 additions & 7 deletions javascript-stringify.js
Original file line number Diff line number Diff line change
Expand Up @@ -223,46 +223,59 @@
* @param {Object} value
* @param {Function} [replacer]
* @param {(Number|String)} [space]
* @param {Object} [options]
* @return {String}
*/
return function (value, replacer, space) {
return function (value, replacer, space, options) {
options = options || {}

// Convert the spaces into a string.
if (typeof space !== 'string') {
space = new Array(Math.max(0, space|0) + 1).join(' ');
}

var maxDepth = options.maxDepth || 200;

var depth = 0;
var cache = [];

/**
* Handle recursion by checking if we've visited this node every iteration.
*
* @param {*} value
* @param {Array} cache
* @return {String}
*/
var recurse = function (value, cache, next) {
var recurse = function (value, next) {
// If we've already visited this node before, break the recursion.
if (cache.indexOf(value) > -1) {
if (cache.indexOf(value) > -1 || depth > maxDepth) {
return;
}

// Push the value into the values cache to avoid an infinite loop.
depth++;
cache.push(value);

// Stringify the value and fallback to
return next(value, space, function (value) {
return recurse(value, cache.slice(), next);
var result = recurse(value, next);

depth--;

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Aug 14, 2016

Author Contributor

For some reason, because of this change the recursion is still infinite.

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 15, 2016

Owner

Huh, really? The issue with the previous code was that it wasn't depth, it was effectively "max values" (always incrementing on each value). Is it possible the example you have is a length combined with depth issue? The test case is passing here, so definitely let me know.

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Aug 15, 2016

Author Contributor

The problem occurs only when having circular references in a custom data type. I added an example to reproduce. You can just clone the repo and run npm i && npm start. When opening http://localhost:3000/, you'll see Uncaught RangeError: Maximum call stack size exceeded in the console with the stack trace.

Maybe you have a solution, so there will be no need to limit the depth by default.

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 15, 2016

Owner

Thanks for the example, I just noticed it's working after I merged 964c5f2. I'm going to review it a little more to figure out why, and release it as 1.3.0 tonight. Thanks again!

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 15, 2016

Owner

Ok, even more interesting, I can default maxDepth = Infinity and it's still working. Perhaps there was some other issue at play here.

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 15, 2016

Owner

Also interesting is the maximum depth we hit on that example is only 14. Pretty far off 100, so I might keep that around just in case.

This comment has been minimized.

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 15, 2016

Owner

Do let me know if there's any other issues you run into 😄

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Aug 15, 2016

Author Contributor

Thanks a lot for all the changes and quick release. Unfortunately, the initial issue is back now - it hangs the browser due to an infinte recursion. It doesn't happen for that object but when having it in another one. I updated the example, uncomment console.log to hang the browser :-D

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Aug 18, 2016

Author Contributor

Hey @blakeembrey,

Just wanted to reassure you got the previous message. It's not in a hurry, as it works well with the previous version removing depth--.

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 19, 2016

Owner

I did, but I haven't been able to check it. I've said this a few times, but removing the decrement is no longer a depth feature, it's a "max elements" feature as all your doing is counting the number of elements to stringify. I can enable that as a flag if it helps and we set it to something fairly high.

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 19, 2016

Owner

Ok, released the max values fix with https://github.com/blakeembrey/javascript-stringify/releases/tag/v1.4.0 - defaults to 100000 and was passing on your test case. Let me know if there's any other troubles, hopefully 100000 values is reasonable 😄

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Aug 19, 2016

Author Contributor

@blakeembrey, thanks a lot for the addition, and sorry for bothering you so much!

If maxDepth doesn't help with infinite recursion, maybe we better set it to Infinity by default?

About maxElements. On one hand, 100000 is reasonable, because 100 like before, could often lead to inconsistent data like in this issue. On the other hand, just tested with Redux Inspector Monitor, and every operation freezes the browser for about 10-20 seconds, making it unusable.

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 19, 2016

Owner

Is there a number that works better we can change it to? I was testing it myself and landed on 100000 because it didn't seem to take too long (maybe a second tops, printing it in the console might take longer). One of the big issues here is that stringification is depth-first and the desired behaviour of maxValues would probably require a refactor to breadth-first. I'm definitely not sure I can do that right now, though I would accept a PR because it would be a more desired behaviour overall.

Neither maxDepth or maxValues are dealing with infinite loops - there's already the stack and encountered arrays for that. If it's in the array, it means we're in a loop. I haven't dived into why these objects you're using are so huge though, there's definitely some crazy generation occurring.

Finally, I actually think you can greatly improve your Redux Inspector without using this library at all. Let me know if you want pointers on doing just that, but I've built rendering processes before that act like Chrome devtools and it's very simple - the only things you really need is Object.getOwnPropertyNames and Object.getPropertyDescriptor, then you can render things dynamically much more efficiently. Here's a project I built three years ago you may be interested in: https://api-notebook.anypoint.mulesoft.com/notebooks. Try it by typing window and hitting "Enter" (related https://github.com/mulesoft/api-notebook/blob/master/src/scripts/lib/stringify.js and https://github.com/mulesoft/api-notebook/blob/master/src/scripts/views/inspector.js).

This comment has been minimized.

Copy link
@blakeembrey

blakeembrey Aug 19, 2016

Owner

Also, it's fine to bother me 😄 I'm glad when someone can get value out of the things I've built, even though it's a very peculiar use-case that I hadn't considered when writing this (I was actually using this to generate code for executing later - I was doing programmatic generation of JavaScript using JavaScript).

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Aug 19, 2016

Author Contributor

Most likely the problem with the Inspector Monitor is more React related, so stringify returns a too big object, which takes a lot to render. Just removing those circular references would help. If there isn't a solution to prevent the infinite loop, then probably will be better to make the default limit smaller (I will investigate).

Neither maxDepth or maxValues are dealing with infinite loops - there's already the stack and encountered arrays for that. If it's in the array, it means we're in a loop. I haven't dived into why these objects you're using are so huge though, there's definitely some crazy generation occurring.

I didn't dig into how openlayers.org API works, just someone reported that our extension hangs the browser. It could be a crazy generation there, though I still cannot understand the object structure.

As understood, the problem is not that the object is huge (we could insert only one element in that array), but because it's infinite.

Funny that we can solve this by doing obj = JSON.parse(jsan.stringify(obj)), before invoking js.stringify(obj). Jsan somehow combines JSON.stringify to get the native object, and handles those circular references. I don't see a way of using this in javascript-stringify. For this specific case, js.stringify(JSON.parse(jsan.stringify(obj))) is 3 times faster than js.stringify(obj).

This comment has been minimized.

Copy link
@zalmoxisus

zalmoxisus Aug 19, 2016

Author Contributor

That project is amazing! Will look into the code when will get some free time.

cache.pop();

return result;
});
};

// If the user defined a replacer function, make the recursion function
// a double step process - `replacer -> stringify -> replacer -> etc`.
if (typeof replacer === 'function') {
return recurse(value, [], function (value, space, next) {
return recurse(value, function (value, space, next) {
return replacer(value, space, function (value) {
return stringify(value, space, next);
});
});
}

return recurse(value, [], stringify);
return recurse(value, stringify);
};
});
33 changes: 27 additions & 6 deletions test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@ var expect = require('chai').expect;
var stringify = require('./');

describe('javascript-stringify', function () {
describe('types', function () {
var test = function (input, result, indent) {
return function () {
expect(stringify(input, null, indent)).to.equal(result);
};
var test = function (input, result, indent, options) {
return function () {
expect(stringify(input, null, indent, options)).to.equal(result);
};
};

describe('types', function () {
describe('booleans', function () {
it('should be stringified', test(true, 'true'));
});
Expand Down Expand Up @@ -106,7 +106,17 @@ describe('javascript-stringify', function () {
var obj = [1, 2, 3];
obj.push(obj);

expect(stringify(obj)).to.equal("[1,2,3,undefined]");
expect(stringify(obj)).to.equal('[1,2,3,undefined]');
});

it('should use multiple references', function () {
var obj = {}
var child = {};

obj.a = child;
obj.b = child;

expect(stringify(obj)).to.equal('{a:{},b:{}}');
});
});

Expand Down Expand Up @@ -210,4 +220,15 @@ describe('javascript-stringify', function () {
expect(string).to.equal('[object Object]');
});
});

describe('max depth', function () {
var obj = { a: { b: { c: 1 } } };

it('should get all object', test(obj, '{a:{b:{c:1}}}'));

it(
'should get part of the object',
test(obj, '{a:{b:{}}}', null, { maxDepth: 2 })
);
});
});

0 comments on commit 8401708

Please sign in to comment.