josephg / node-foundationdb

Modern Node.js FoundationDB bindings
Other
115 stars 17 forks source link

Losing KeyIn/KeyOut type parameters when using directories #70

Closed NuckChorris closed 1 year ago

NuckChorris commented 1 year ago

Whenever I use .at with a directory, it loses my key/value encoders, see following including the types reported in VS Code:

import fdb from 'foundationdb';

fdb.setAPIVersion(620);

const rootDb = fdb.open().withKeyEncoding(fdb.tuple);

const directorySubspace = rootDb.at(await fdb.directory.createOrOpen(rootDb, 'test'));
// const directorySubspace: fdb.Database<NativeValue, Buffer, NativeValue, Buffer>

const stringSubspace = rootDb.at('test');
// const stringSubspace: fdb.Database<string | number | boolean | Buffer | BigInt | TupleArr | {
//     type: "uuid";
//     value: Buffer;
// } | {
//     type: "float";
//     value: number;
//     rawEncoding?: Buffer | undefined;
// } | ... 5 more ... | undefined, fdb.TupleItem[], NativeValue, Buffer>

As you can see, it works fine if my subspace is a string prefix, but a directory prefix seems to break the type and lose my key/value encoders :(

NuckChorris commented 1 year ago

Actually these types may be accurate, it seems like .at with a directory resets the encoders, which seems... odd? Is this behavior intentional?

josephg commented 1 year ago

Yes, it is odd - and its certainly not ideal. When I wrote it I wasn't sure what the behaviour should be in this case, and the directory layer code is mostly ported directly from the python bindings to keep things simple. I'm having a re-read of the code to see how it could be improved. It seems like a pretty obvious fix to plumb the KV transformers through.

The problem is when you call DirectoryLayer#createOrOpen like this:

fdb.directory.createOrOpen(rootDb, 'test')

It doesn't inherit the KV transformers from the passed database.

The relevant code is here. If you follow that down, contentSubspaceForNode should take an optional subspace and then we can plumb your rootDb through.

You're welcome to put together a PR. We'll have to bump the major version number because it'll be a semver-breaking change.

josephg commented 1 year ago

... I've thrown together the first pass of a fix for this. I don't like the template parameters everywhere, but I think its the right solution. Your example should work with the new code.

josephg commented 1 year ago

... And I published it as foundationdb@2. Give it a try and let me know if there are any problems. Windows support has been removed for now due to this issue.