Skip to content
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

Handle circular references better #75

Open
fregante opened this issue Dec 17, 2023 · 2 comments
Open

Handle circular references better #75

fregante opened this issue Dec 17, 2023 · 2 comments

Comments

@fregante
Copy link

fregante commented Dec 17, 2023

It looks like #44 isn't fully fixed.

It's hard to make a repro because I'm using loupe via Vitest, but it seems to be a problem with https://github.com/WebReflection/linkedom and some documents:

The repro should look something like:

import {parseHTML} from 'linkedom';

const request = await fetch('https://github.com/refined-github/refined-github/issues', {
	headers: {
		Accept: 'text/html',
	},
});
const html = await request.text();
const {document} = parseHTML(html);
const matches = document.querySelectorAll('a');

assert.lengthOf(matches, 100)

The assertion probably gets caught up trying to preview the DOM objects, which has plenty of circular references. I'm not sure what's wrong with the DOM generated by that particular page, but it fails there and not on other similar pages.


RangeError: Maximum call stack size exceeded
 ❯ inspectProperty node_modules/loupe/loupe.js:254:27
 ❯ inspectList node_modules/loupe/loupe.js:208:28
 ❯ inspectObject node_modules/loupe/loupe.js:575:28
 ❯ inspectClass node_modules/loupe/loupe.js:606:35
 ❯ Object.inspect node_modules/loupe/loupe.js:850:16
 ❯ inspectProperty node_modules/loupe/loupe.js:268:21
 ❯ inspectList node_modules/loupe/loupe.js:208:28
 ❯ inspectObject node_modules/loupe/loupe.js:578:26
 ❯ Object.inspect node_modules/loupe/loupe.js:838:14
 ❯ inspectProperty node_modules/loupe/loupe.js:268:21
@koddsson
Copy link
Member

koddsson commented Jan 6, 2024

Oh wow, let me take a look at this. Ping me if I forget. I can at least reproduce it so that's something:

  1) assert
       lengthOf doesn't crash:
     RangeError: Maximum call stack size exceeded
      at inspectObject (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/object.js:2:23)
      at inspectClass (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/class.js:13:20)
      at Object.inspect (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/index.js:118:14)
      at inspectProperty (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/helpers.js:144:19)
      at inspectList (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/helpers.js:108:28)
      at inspectObject (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/object.js:14:28)
      at inspectClass (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/class.js:13:20)
      at Object.inspect (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/index.js:118:14)
      at inspectProperty (file:///Users/cr91ew/src/koddsson/chai/node_modules/loupe/lib/helpers.js:144:19)

@fregante
Copy link
Author

fregante commented Jan 7, 2024

When generating the preview, would it make sense to automatically limit the size?

For example it's useful to see the message Expected [0, 2, 8] to have length 4, but it becomes unreadable when the array is huge or when it contains large objects.

An example limit for this specific case could look like arr.length * Object.keys(arr[0]).length < 20, although I know it might be difficult to translate it into real code. This might also be a choice of the caller rather than loupe's itself.

I know that Node's inspect won't try to go too deep into objects, so such a limit alone should solve circular references permanently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants