Callidon / bloom-filters

JS implementation of probabilistic data structures: Bloom Filter (and its derived), HyperLogLog, Count-Min Sketch, Top-K and MinHash
https://callidon.github.io/bloom-filters/
MIT License
360 stars 41 forks source link

saveAsJSON() method not found #20

Closed stbrody closed 3 years ago

stbrody commented 3 years ago

The README says that you can call .saveAsJSON() on any bloom filter to get a serialization of the bloom filter state that can be persisted or sent over the network. When I try to use it, however, my Typescript code fails to compile with the error: error TS2339: Property 'saveAsJSON' does not exist on type 'BloomFilter'.

My code looks like:

import { BloomFilter } from 'bloom-filters'

// [...]
const bloomFilterEntries = new Set<string>()
// [...]

const bloomFilter = BloomFilter.from(bloomFilterEntries, 0.01)
const bloomJSON = bloomFilter.saveAsJSON()
stbrody commented 3 years ago

When I look at the bloom-filter.d.ts file within my node_modules that was set up by NPM, I don't see a saveAsJSON method in the BloomFilter class. Similarly, looking at the bloom-filter.ts source file, I don't see one there either. So it makes sense that the node compiler would fail to find that method since it doesn't seem to exist.

I do however see some logic related to saveAsJSON in the exportable.ts file in the source. Is there something special I need to do to get my BloomFilter instance to register as an Exportable?

Callidon commented 3 years ago

The method exists for all classes in the package, but I think the latest rework might have removed the method from the definition files, which causes your TypeScript compiler to fail.

I can't promise that we can provide a fix quickly, because we now have very little time to work on open source projects (sadly). But I will definitively look into it, because that 's a rather nasty bug !

folkvir commented 3 years ago

@Callidon I think adding saveAsJSON and fromJSON into the base-filter as simple abstract/interface methods will solve our problem. Those methods will be modified by the auto-exportable decorator anyway.

folkvir commented 3 years ago

@stbrody Please, can you test the following branch? https://github.com/Callidon/bloom-filters/tree/fix_saveasjson Everything should work correctly.

stbrody commented 3 years ago

@folkvir @Callidon, sorry I just got to looking at this again. I tried upgrading the package to version 1.3.2 and things are actually worse now. There's no longer a compilation error on saveAsJSON or fromJSON, but now any time I try to call the .has() method on a filter parsed out from fromJSON I get a new compilation error:

src/merkle/__tests__/merkle-metadata.test.ts:39:24 - error TS2339: Property 'has' does not exist on type 'void'.

Looks like because fromJSON doesn't specify a return type the TS compiler assumes it returns nothing. It probably needs to specify that it returns a BloomFilter. I assume a similar problem most likely exists for saveAsJSON() as well.

folkvir commented 3 years ago

Well, do you still have the issue with this PR? https://github.com/Callidon/bloom-filters/pull/22 The problem is that the method is overrided by a templated decorator which actually returns the correct object but is detected by the compiler as a void one. It should now be a Basefilter which can be easily casted as a BloomFilter as explained in the PR. But it still requires you to cast the object in the right format.

stbrody commented 3 years ago

Well, do you still have the issue with this PR? #22 The problem is that the method is overrided by a templated decorator which actually returns the correct object but is detected by the compiler as a void one. It should now be a Basefilter which can be easily casted as a BloomFilter as explained in the PR. But it still requires you to cast the object in the right format.

Yeah, I guess that makes it a little better, but it would be nice not to have to cast at all. If BaseFilter actually had definitions of the common bloom filter functions (add, has, remove) that are shared by all the filter implementations then you could use what is returned from fromJSON directly without caring about what underlying filter type is being used.

folkvir commented 3 years ago

I agree with your point of a non-casting obligation but BaseFilter is a super class defining seeding properties of every bloom filters. It does not have any specific method. So I need to find another method dealing with decorators and templates.

folkvir commented 3 years ago

@Callidon @stbrody Unfortunately with our templating methods and decorators we cannot make a small change without redefining fromJSON in all Filters of the project. So in the last commit I defined the return type of fromJSON to any this way you can have type definitions only in two ways.

This is the only solution for us.

Working example:

import { BloomFilter } from '../../bloom-filters/dist/api'

// [...]
const bloomFilterEntries = new Set<string>(["test"])
// [...]

const bloomFilter: BloomFilter = BloomFilter.from(bloomFilterEntries, 0.01)
const bloomJSON: Object = bloomFilter.saveAsJSON()
console.log('JSON: ', bloomJSON)
const b: BloomFilter = BloomFilter.fromJSON(bloomJSON)
console.log('B: ', b)
console.log(b.has('test'))

PS: Just a reminder if you are not aware of it, don't even try to test or add data on a Filter that are initialized with an empty set or array. It will cause RangeError: Maximum call stack size exceeded. T.from functions are used to initialize filters with predefined values. Otherwise use the classic constructor or the T.create function to initialize a filter of fixed size with desired error rate.

folkvir commented 3 years ago

@stbrody You can upgrade to 1.3.3. Thank you @Callidon