Conversation
|
A few adjustments are still needed for the v4 migrations. @tim-smart what's the @gcanti |
lewxdev
left a comment
There was a problem hiding this comment.
This is a non-exhaustive feedback on the current changes.
@tim-smart please feel free to review these comments before @SandroMaglione makes any changes
| /** @internal */ | ||
| const IDBFlatKey = Schema.Union([ | ||
| Schema.String, | ||
| Schema.Number, | ||
| Schema.Date, | ||
| Schema.declare( | ||
| (input): input is BufferSource => | ||
| input instanceof ArrayBuffer || ArrayBuffer.isView(input), | ||
| ), | ||
| ]); | ||
|
|
||
| /** | ||
| * Schema for `IDBValidKey` (`number | string | Date | BufferSource | IDBValidKey[]`). | ||
| * | ||
| * @since 1.0.0 | ||
| * @category schemas | ||
| */ | ||
| export const IDBValidKey = Schema.Union([IDBFlatKey, Schema.Array(IDBFlatKey)]); |
There was a problem hiding this comment.
- This can be simplified by using
Schema.suspendto define the recursive schema. - According to the standard (IndexedDB API 3.0), only a subset of the types provided are valid keys. Notably these types differ:
number(excludingNaN)Date(excluding any invalidDate)
- Effect 4.0 does not currently have an equivalent for
Schema.NonNaN, aSchema.isNonNaNfilter could be added to further simplify this check. Schema.mutableis used because the built-in DOM declaration definesIDBValidKeywith a mutable typeIDBValidKey[]. Alternatively, a type may be defined withreadonly IDBValidKey[](Readonly<IDBValidKey>errors)
| /** @internal */ | |
| const IDBFlatKey = Schema.Union([ | |
| Schema.String, | |
| Schema.Number, | |
| Schema.Date, | |
| Schema.declare( | |
| (input): input is BufferSource => | |
| input instanceof ArrayBuffer || ArrayBuffer.isView(input), | |
| ), | |
| ]); | |
| /** | |
| * Schema for `IDBValidKey` (`number | string | Date | BufferSource | IDBValidKey[]`). | |
| * | |
| * @since 1.0.0 | |
| * @category schemas | |
| */ | |
| export const IDBValidKey = Schema.Union([IDBFlatKey, Schema.Array(IDBFlatKey)]); | |
| /** | |
| * @since 1.0.0 | |
| * @category schemas | |
| */ | |
| export const IDBValidKey: Schema.Schema<IDBValidKey> = Schema.Union([ | |
| Schema.String, | |
| Schema.Number.check(Schema.makeFilter((input) => !Number.isNaN(input))), | |
| Schema.DateValid, | |
| Schema.declare((input): input is BufferSource => | |
| input instanceof ArrayBuffer || | |
| ArrayBuffer.isView(input) && input.buffer instanceof ArrayBuffer | |
| ), | |
| Schema.mutable(Schema.Array(Schema.suspend(() => IDBValidKey))), | |
| ]); |
There was a problem hiding this comment.
You can use Schema.Finite for finite numbers. I think it excludes nan too
There was a problem hiding this comment.
I would avoid using suspend here and keep it as it was.
There was a problem hiding this comment.
You can use Schema.Finite for finite numbers. I think it excludes nan too
Schema.Finite would be too restrictive since it excludes Infinity and -Infinity, which are valid keys according to the standard.
I would avoid using suspend here and keep it as it was.
No suspend is fine as it's functionally equivalent.
There was a problem hiding this comment.
OK if infinite is permitted then the nan check is fine.
suspend works, but you end up with a better schema ast without it.
There was a problem hiding this comment.
IndexedDB is effectively a NoSQL database, and I think using vocabulary like "Table" is incorrect. This module would be more accurate renamed to IndexedDbObjectStore. ("Table" makes sense if you have columns)
Note that the "Table" vocabulary is also used throughout and should be updated as well.
| export const AutoIncrement = Schema.Number.annotate({ | ||
| identifier: "AutoIncrement", | ||
| title: "autoIncrement", | ||
| description: "Defines a valid autoIncrement key path for the IndexedDb table", | ||
| }); |
There was a problem hiding this comment.
According to the standard (IndexedDB API 3.0), the autoIncrement key generator must be a positive integer not greater than 253. That check has been added here.
| export const AutoIncrement = Schema.Number.annotate({ | |
| identifier: "AutoIncrement", | |
| title: "autoIncrement", | |
| description: "Defines a valid autoIncrement key path for the IndexedDb table", | |
| }); | |
| export const AutoIncrement = Schema.Number | |
| .check(Schema.isInt(), Schema.isBetween({ minimum: 1, maximum: 2 ** 53 })) | |
| .annotate({ | |
| identifier: "AutoIncrement", | |
| title: "autoIncrement", | |
| description: "Defines a valid autoIncrement key path for the IndexedDb table", | |
| }); |
There was a problem hiding this comment.
Schema.Int.check(Schema.isBetween({ minimum: 1, maximum: 2 ** 53 }))| export interface IndexedDb { | ||
| readonly [TypeId]: TypeId; | ||
| readonly indexedDB: globalThis.IDBFactory; | ||
| readonly IDBKeyRange: typeof globalThis.IDBKeyRange; | ||
| } |
There was a problem hiding this comment.
Based on other Effect modules, it may make more sense to expose effectful service functions rather than the built-in interface. For example, as a rough sketch:
export interface IndexedDbFactory {
readonly open: (name: string, version: number) => Effect.Effect<...>;
readonly deleteDatabase: (name: string) => Effect.Effect<...>;
readonly cmp: (first: IDBValidKey, second: IDBValidKey) => Effect.Effect<...>;
readonly databases: Effect.Effect<...>;
}
export interface IndexedDbKeyRange {
readonly only: (value: IDBValidKey) => Effect.Effect<...>;
readonly lowerBound: (lower: IDBValidKey, open?: boolean) => Effect.Effect<...>;
readonly upperBound: (upper: IDBValidKey, open?: boolean) => Effect.Effect<...>;
readonly bound: (lower: IDBValidKey, upper: IDBValidKey, lowerOpen?: boolean, upperOpen?: boolean) => Effect.Effect<...>;
}There was a problem hiding this comment.
I would keep the service lean and then potentially expose another service that further extends it with effectful methods. Keeps it easily injectable for tests etc
There was a problem hiding this comment.
The effectful service could be added at a later date.
There was a problem hiding this comment.
Correct me if I'm wrong, but with the current implementation it's not possible to create an index or key on a deeply nested field.
This is a big limitation that could be addressed by taking an annotation-driven approach to defining ObjectStore schemas (although this may not work well with the query builder unless some TypeScript wizardry is employed, but maybe that's an okay tradeoff).
Type
Description
Porting of the
IndexedDbPR from v3 to v4 (original source).