fix(diff-snapshot): Fix stringify failure due to circular referneces. #72
Conversation
|
|
|
@skywhale does this solve the node 9 compatibility issues? I thought the issue was with the pngjs package 🤔 If this solves them then can you modify the travis config to add node 9 so that we know before this PR is merged if that is truly the case and so we don't suffer from any regressions on 9? |
|
There are two separate issues, one in pngjs, and another in jest-image-snapshot. I merged pngjs/pngjs#108 to my local repo, and started seeing this TypeError due to a circular ref introduced in PNG. Both PRs need to land. |
|
@anescobar1991 Added v9 to travis config. Let me get back to this PR after pngjs/pngjs#110 is merged. |
|
@skywhale thanks! |
|
I wonder why the tests passed on node 9 though? I know the integration tests were failing for me on node 9 before. Edit: never mind I see that it is because you have it referencing the branch with the bugfix |
|
or because pngjs is fixed now! 🎆 |
package.json
Outdated
| @@ -55,7 +55,7 @@ | |||
| "lodash": "^4.17.4", | |||
| "mkdirp": "^0.5.1", | |||
| "pixelmatch": "^4.0.2", | |||
| "pngjs": "^3.3.0", | |||
| "pngjs": "https://github.com/skywhale/pngjs/archive/fixnode9.tar.gz", | |||
There was a problem hiding this comment.
change this back and it should work now!
There was a problem hiding this comment.
@skywhale LGTM, can you squash your commits? (squashed on merge)
|
Awesome! Any chance we can cut a new version to npm with this change? |
|
@dmk255 done! v2.4.1 includes these changes |
…mericanexpress#72) * fix(diff-snapshot): Fix stringify failure due to circular referneces. * fix(diff-snapshot): Apply tslint --fix * Add node v9 to CI build * DO NOT SUBMIT: Fix pngjs. * Remove comment. * Upgrade pngjs to 3.3.3
Since node 9, PNG._packer._handle.jsref has a circular reference, causing JSON.stringify to fail. Since none of the hidden properties starting with _ aren't needed for writing the images, I went ahead and discarded them all.