jakearchibald / idb

IndexedDB, but with promises
https://www.npmjs.com/package/idb
ISC License
6.31k stars 356 forks source link

TypeScript: special case for auto-incrementing primary keys? #150

Open dumbmatter opened 4 years ago

dumbmatter commented 4 years ago

For object stores with auto incrementing primary keys, we know the primary key will be available on all objects read from the database, but not necessarily on all objects written to the database. Currently idb does not supply TypeScript with that information, though. Seems to me that it could be improved to do so, if DBSchemaValue also included the keyPath and autoIncrement values passed to createObjectStore.

Example:

type Foo = {
    id?: number;
    bar: string;
}

export interface FooDB extends DBSchema {
    foos: {
        key: number;
        value: Foo;
    };
}

const db = await openDB<FooDB>("foo", 1, {
    upgrade(db, oldVersion) {
        if (oldVersion === 0) {
            upgradeDB.createObjectStore("leagues", {
                keyPath: "id",
                autoIncrement: true,
            });
        }
    },
});

// This works, only because we defined Foo with an optional id property
const foo: Foo = {
    bar: "baz",
};
await db.add("foo", foo);

// foo2 is guaranteed to have a numeric id property, but TypeScript doens't know this because it has type Foo
const foo2 = await db.get("foo", 1);

// Ideally, foo2's type would instead be:
type Foo2 = Foo & { id: number }

// ...but that would require encoding in FooDB that id is an autoincrementing primary key.

I guess a caveat would be that if keyPath is nested like a.b.c I'm not sure if TypeScript will be able to handle that. But based on pure conjecture and absolutely no data, I'm going to say that it's very rare to use a key path that is both nested and auto incrementing, but it's pretty common to use a non-nested key path that is autoincrementing.

jakearchibald commented 4 years ago

Interesting! I'll have a think about this one. I wonder if I can create a type like AutoIncrementingKey which is optional on add but required on put, and always present on objects returned.

dumbmatter commented 4 years ago

IIRC it'd still be optional on put.

I'm going to give it a shot and hopefully send you a PR if I can make it work. Looks like it is possible to conditionally add a required field to a type:

type AutoIncrementKeyPath = { autoIncrementKeyPath: string };
type WithID<T extends {} | AutoIncrementKeyPath> = T & (T extends AutoIncrementKeyPath ? Record<T["autoIncrementKeyPath"], number> : {});

type Foo = {
    id?: number;
    bar: string;
}

type FooDB = {
    autoIncrementKeyPath: "id";
}
type FooDB2 = {}

type FooWithID = WithID<FooDB>;

type FooWithoutID = WithID<FooDB2>;
dumbmatter commented 4 years ago

After further consideration, I think I have a better idea for how to solve this issue.

Recapping the old ideas:

Idea 1: My original idea was problematic because it will not work for nested keys.

Idea 2: Your idea from this comment in my PR IMHO is not ideal because it requires users to think about idb when writing types for their objects. In general, I think people don't write these types specifically for idb. They write them because these objects are used throughout their application. It seems too invasive to suggest that we need to use these special types from idb.

But there are other options too!

Idea 3: Tweak idea 1 to make it work for nested keys. So instead of:

autoIncrementingStore: {
  key: number;
  value: {
    id?: number;
    name: string;
  };
  autoIncremetKeyPath: 'id';
};

do this:

autoIncrementingStore: {
  key: number;
  value: {
    id?: number;
    name: string;
  };
  autoIncremetKeyPath: { id: number };
};

Then similar to my original idea, use value for any object written to IndexedDB, and use the intersection of value and autoIncrementKeyPath for any value returned from IndexedDB. That will work for an arbitrarily nested key path.

Idea 4: Same as idea 3, but have users specify the whole object.

autoIncrementingStore: {
  key: number;
  value: {
    id?: number;
    name: string;
  };
  valueWithKey: {
    id: number;
    name: string;
  };
};

This seems like a clunkier version of idea 3, since it forces you to be redundant, right?

My concern is the same as I wrote above about idea 1 - I assume people don't write these types specifically for idb, they write them because they use them in general in their application. So it would actually look like:

autoIncrementingStore: {
  key: number;
  value: Value;
  valueWithKey: ValueWithKey;
};

Functions which don't care about id would use Value, and functions that do care about id would use ValueWithKey. And I think most non-trivial applications would have both types of functions. If users are going to create these types anyway, we might as well use them directly in idb.


From my selfish perspective as a user of idb, ideas 1, 3, and 4 would be perfectly acceptable for my purposes. Idea 2 might be a little annoying, but workable.

What are your thoughts? And if you'd like a PR for idea 3 or 4, let me know, I'd be happy to do it.

ericbf commented 4 years ago

What are your thoughts? And if you'd like a PR for idea 3 or 4, let me know, I'd be happy to do it.

Ran into this today. Seems to me that your idea 4 would be the most simple and least "automagical", reducing error-proneness. Users can reduce redundancy themselves if they want to not repeat the type with something like this:

autoIncrementingStore: {
  key: number;
  value: Value;
  valueWithKey: Value & { path: { to: { key: number } } };
};

Which would resolve to a version of Value where the key is required. Best to keep it simple!

Might I suggest, however, that the keys be named something more precise? Perhaps "valueFromGet" or something? Both values would have the key prop, it's just that it would be required in one but not the other.

I'm not the person you were asking, but I'd very much like a PR for option 4!

Of course, the same could apply to option 3. This would essentially be a different way to represent the above:

autoIncrementingStore: {
  key: number;
  value: Value;
  autoIncrementKeyPath: { path: { to: { key: number } } };
};

Then users could extract the types as so:

export type ValueWithKey = Value & Schema["autoIncrementingStore"]["autoIncrementKeyPath"]

So I guess I could go either way. But a PR for either would be greatly appreciated!

dumbmatter commented 4 years ago

I've been using my fork which includes my original idea (autoIncrementKeyPath: 'id') and works fine for my purposes. Feel free to use it if you want! I'm probably going to hold off on further work unless @jakearchibald says he wants to include it in idb. But I agree, option 4 sounds best to me now.

jakearchibald commented 4 years ago

I wonder if there's a way in TypeScript to 'subtract' autoIncrementKeyPath from value, leaving a type that's missing the key, which can be accepted on add.

ericbf commented 4 years ago

@jakearchibald yes, but it's not as simple.

A mapped type like this could accomplish a "subtracting" of the key as opposed to an adding of it:

type OmitNested<Type, PathToKey> = PathToKey extends object ? Omit<Type, keyof PathToKey> & {
    [Key in Extract<keyof PathToKey, keyof Type>]: OmitNested<Type[Key], PathToKey[Key]>
} : PathToKey extends keyof Type ? Omit<Type, PathToKey> & { [Key in PathToKey]?: Type[Key] } : never

type Type = {
    name: string
    anotherType: {
        value: string
    }
    path: {
        to: {
            key: number
            notKey: string
            anotherNotKey: string
        }
        notTo: {
            somethingElse: string
        }
    }
}

type WithoutKey = OmitNested<Type, { path: { to: "key" } }>

const withoutKey: WithoutKey = {
    name: "Hi!",
    anotherType: {
        value: "HEllo"
    },
    path: {
        to: {
            notKey: "Hi",
            anotherNotKey: "Hi2"
        },
        notTo: {
            somethingElse: "Hi"
        }
    }
}

Here's a playground with the above.

jakearchibald commented 3 years ago

I wonder if some of the string-processing TypeScript stuff could help us here.

meticoeus commented 3 years ago

Is there some way to handle adding objects on a strongly typed store with an autoincrementing key in typescript currently?

tx.store.add(objectWithoutKey); // <- TS compiler error: objectWithoutKey missing required property "id"

Or are we stuck with @ts-ignore comments until this issue has a resolution?

Also: Nice work on this library. It has made learning to use and using IDB less annoying since the API is mostly the same : ) .

dumbmatter commented 3 years ago

@meticoeus I'm still using a fork with my original idea on how to solve it https://github.com/dumbmatter/idb/tree/bbgm2 see the stuff about autoIncremetKeyPath in the README. It works fine for my purposes, feel free to use it if you'd like.

meticoeus commented 3 years ago

@dumbmatter Thanks for the suggestion. I'm solving for now by isolating obj construction in a function with an output type that omits the id and passing the result with casts

ts.store.add(buildDbObjWithoutKey(args) as unknown as DbObjWithKey);

...

function buildDbObjWithoutKey(args): Omit<DbObjWithKey, "id"> {
  return {...};
}

Its a little messy but mostly keeps type checking in place.

dumbmatter commented 2 years ago

I wonder if some of the string-processing TypeScript stuff could help us here.

I think it can. Check this out:

const stringX = "a.b";
const stringY = "a.b.c.d";

type StringToNestedObject<S extends string, KeyType extends unknown> =
    S extends `${infer T}.${infer U}` ? {[Key in T]: StringToNestedObject<U, KeyType>} : {[Key in S]: KeyType};

type TypeX = StringToNestedObject<typeof stringX, string>;
// TypeX is: { a: { b: string; } }

type TypeY = StringToNestedObject<typeof stringY, number>;
// TypeY is: { a: { b: { c: { d: number; } } } }

Maybe I'll try to make another PR based on this approach...

Edit: Combined with OmitNested from @ericbf above...