Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

"circular structure" error with v3.7.0 #11349

Closed
benoitvallon opened this issue Jan 30, 2016 · 10 comments
Closed

"circular structure" error with v3.7.0 #11349

benoitvallon opened this issue Jan 30, 2016 · 10 comments

Comments

@benoitvallon
Copy link

I have the following error: Converting circular structure to JSON with npm v3.7.0 (node 5.5.0). I think I get the idea behind the "circular structure" message, but when there are a lot of dependencies in a project I wonder how it is possible to avoid that error. To me it seems impossible to check manually the dependencies of dependencies of my project.

You can see here on travis that previous npm versions work well. The project is there https://github.com/benoitvallon/react-native-nw-react-calculator

npm-debug.log.zip

Since v3.3.10 I had this issue #10985 and I had to run npm install 2 times but now that's an other error.

@4kochi
Copy link

4kochi commented Jan 30, 2016

I get the same when trying to install the module npm-check-updates.

chuck:~ chuck$ sudo npm i -g npm-check-updates
npm ERR! Darwin 15.3.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "i" "-g" "npm-check-updates"
npm ERR! node v5.5.0
npm ERR! npm  v3.7.0

npm ERR! Converting circular structure to JSON
npm ERR! 
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/chuck/npm-debug.log
chuck:~ chuck$ sudo npm i -g npm-check-updates
npm ERR! Darwin 15.3.0
npm ERR! argv "/usr/local/bin/node" "/usr/local/bin/npm" "i" "-g" "npm-check-updates"
npm ERR! node v5.5.0
npm ERR! npm  v3.7.0

npm ERR! Converting circular structure to JSON
npm ERR! 
npm ERR! If you need help, you may report this error at:
npm ERR!     <https://github.com/npm/npm/issues>

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/chuck/npm-debug.log

@4kochi
Copy link

4kochi commented Jan 30, 2016

Might be caused by 1d1ea7e

@othiym23
Copy link
Contributor

It almost certainly is, @4kochi. Probably best to revert to npm@latest until we have a chance to take a look at this early next week and put out a new test version that fixes this issue. It's always something!

@4kochi
Copy link

4kochi commented Jan 30, 2016

Yeah, I know. Will switch back to 3.6.0 for now.

@jdalton
Copy link
Contributor

jdalton commented Jan 30, 2016

If a package is just json why is there a circular reference? Is the package object getting modified somehow or is the clone method being used on non json objects? @STRML

@STRML
Copy link
Contributor

STRML commented Jan 30, 2016

That's very odd, there shouldn't be circular references. I think that's the real bug here, but we should do some investigation.

@othiym23
Copy link
Contributor

That's very odd, there shouldn't be circular references.

Circular references are quite common, because many of the objects being cloned by that function have been run through read-package-json when the package.json is cached on disk, and therefore they've been run through the full set of canonicalization and transformation operations of read-package-json and normalize-package-data, which isn't written to avoid cycles. In particular, it looks like the package objects coming from regCache frequently have packages pointing back to themselves via the versions object. (I had to do some spelunking and instrumentation to figure this out, but I believe my understanding is correct.)

The short-term fix is to revert 1d1ea7e – the architectural work necessary to make it safe to use JSON.parse(JSON.stringify()) here is significant, and @iarna and I already know we want to rework fetchPackageMetadata to the point that it no longer exists. If you want to take a stab at preserving the current behavior or coming up with an alternate solution that preserves the performance gains of this patch, working up a test case that fails against npm@3.7.0 and passes with the new strategy is probably a prerequisite. I haven't had a chance to reduce it to a minimum repro case, but

npm -g install aws-sdk babel-cli bower documentation gulp-cli jscs jshint rimraf which rimraf sitespeed.io tap vmd

will reliably crash for me.

@iarna
Copy link
Contributor

iarna commented Jan 30, 2016

A correction would be something like lib/install/copy-tree.js where it knows what it needs to actually deep-copy and what's immutable.

@jdalton
Copy link
Contributor

jdalton commented Jan 30, 2016

A correction would be something like lib/install/copy-tree.js where it knows what it needs to actually deep-copy and what's immutable.

Nice! I was thinking if the properties that contain circular references are known the clone could temporarily null them then perform the json clone and then restore the properties with circular references with a lodash.cloneDeep. Then see if there's perf wins.

@STRML
Copy link
Contributor

STRML commented Jan 31, 2016

Sounds like there should be perf wins here if we can isolate exactly what needs to be defensively cloned and deal with that data specifically, or eliminate the need for cloning.

@othiym23 or @iarna, can you speak to why the clone exists at all?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.