search

jaredwray / flat-cache

A stupidly simple key/value storage using files to persist the data
MIT License
165 stars 30 forks source link

FIX: Use circular-json to handle circular references (fix #5) #6

Closed nopnop closed 8 years ago

nopnop commented 8 years ago

Ok, I found that the problem illustrated in #5 is not related to circular dependency in js module. The real reason is the use of the browserify.transform option field in a node package when used with watchify (circular reference with packageCache).

I made a small project to illustrate the issue: https://github.com/nopnop/fix-circular-flatcache

Using the suggested https://github.com/WebReflection/circular-json for both serializing and deserializing fix the issue nicely.

The test passes: yet I'm not sure how to handle this special case of Circular Reference in the test suite.

The stateful fileCache variable I've added is used to reproduce the caching mechanism of the removed read-json-sync package

royriojas commented 8 years ago

Very nice!, Since this is also used by eslint I guess I will consider to publish this under a major version to avoid breaking it, until we have a chance to test it properly.

About how to add a test, you're right it might not be that simple, but I guess that the easier will be to create a circular structure and assigning it to a key, and then attempt to serialize it.

Once you add the test, I will be very happy to merge this. Thank you so much for your time and effort.

royriojas commented 8 years ago

There is just a minor suggestion and I will merge. This is really awesome!

nopnop commented 8 years ago

Thanks! I was on the go yesterday: I haven't took the time to explain my last commits..

I'll push a fix on the readJSON (see my answer above).

royriojas commented 8 years ago

Hi @nopnop

I really appreciate the help, thank you for your PR it was merged already. Will publish this soon.

royriojas commented 8 years ago

Hi @nopnop, it is published now as flat-cache@1.2.0, can you do a PR to persistify to update the dependency?

Thank you.

nopnop commented 8 years ago

Hi @royriojas,

I've made a bump PR.

Many thanks! (And sorry for the missed files field )