Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v2.0.0 #90

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

v2.0.0 #90

wants to merge 33 commits into from

Conversation

toptobes
Copy link
Collaborator

No description provided.

toptobes and others added 20 commits September 29, 2024 20:26
* remove deprecated vector/vectorize parameters

* remove bulkWrite

* remove deleteAll

* removed namespace terminology

* removed db.collections()

* removed client.db(id, region)

* update api report
* some experimental table typing

* dark magic

* added default type for table

* made InferTableSchema more flexible

* moved table.ts into its own folder (mirrors collections)

* more type errors

* broke up table.ts file in to its proper structure

* more work on tavles idk

* start work on common command impls class

* fixed couple rebasing errors

* moved all collection functions to generic internal CommandsImpls object

* fixed all of the bugs I introduced in the previous commit :)

* more implmeennation work

* added table methods for db as well

* added types (but not impl) for alterTable

* add countRows

* add more missing table functions

* added various datatypes (dates & ip addrs)

* update build report

* update build report

* some datatype tweaks (mostly for InetAddress)

* some type operations when translating cql types
* various cursor tweaks & fixes

* minor typing tweaks

* fixed bug with filter being potentially muitable

* made cursors immutable

* some tests not all idk
* started work overhauling validation logic

* so much validation

* some folder resturcuting

* loggign hierarchy

* basic logging impl

* implement warning events

* tests for logging and such
* documented table-schema file

* tsdoc work

* remove checkExists

* ser/des work

* playground script

* started adding custom inspects and work on datetimes

* made _httpClient fully public

* work on DataAPIVector stuff

* some resturcturing

* made all Promise<true> methods just reutrn Promise<void>

* removed rackstackk hack

* cqlblob type

* additionalHeaders

* started documenting table-related stuff

* formatting for events logging

* documentation for logging

* document collection class

* created CursorError

* set up test suite for table tests

* move dropIndex to db level

* lot of work on bignumbers hack...

* remove CollectionNotFoundError

* added table.deifnition()

* super basic insertOne test

* basic findone tests

* toomanyrowstocount error

* start documenting serdes

* split cumulative errors + some more bignumber serdes work

* changed $PrimaryKeyType to be a string + some test typing fixes

* added sparse data support
* make DataAPIDbAdmin keyspace options no longer extend AdminBlockingOptions

* refactor raw db info into more workable objects

* more unified naming convention of admin interfaces

* bit of name tweaking

* some admin info itnerface tweaking

* light, temp documentation
* split cursor classes

* little bit of cleanup

* moved cursors test to documents/collections

* unit tests for split cursors

* integration tests for split cursors
* timeouts overhaul

* keyspace impl & tests

* more tests & tweaks

* fix timeout/sort bgus

* refined WithTimeout types

* createCollection custom timeout impl

* more docs and stuff
* a bunch of tests work

* couple minor test fixes
* more intuitive naming for events/logging stuff

* formatting + timestamps for log messages

* dropIndex ifExists

* sourceModel

* createindex options resturcuting

* split filter & update types

* remove cql from datatypes names

* timeout for cursor.toArray() & coll.distinct()

* added class names for admin event name sourcse

* remove "spawn" from spawn type names
* updated serdes

* changed tokne provider a bit

* minor linting fix

* fixed couple import/typing issues + tables readme

* add shorthand datatype functions

* update readme w/ shorthand datatype fns

* marked mroe internal values

* removed deeppartial & strict filters/sorts/projs

* tiny bit of renaming
* updated serdes

* add shorthand datatype functions

* update readme w/ shorthand datatype fns

* marked mroe internal values

* start documentation of tables

* added listIndexes

* some altertable fixes

* bump min node version to v18+
* advanced typings for tables/colls; typings for includeSimialrity

* fixed tables typing test file

* update readme
* switch to codec-based ser/des system

* camel snake case interop

* example and many bug fixes and tweaks and stuff idk lol

* added serdes path matching & class-mapping example
* some fixes/work

* some ttyping work

* added tsdoc everyuwhere for the msot part

* reset example astardbts versions

* few more tests

* minor uipdates to examples
* update uuid stuff + start datatypes tests

* reexport bignumber

* some datatype tests
* unficiation of codec types

* snakeCaseInterop => keyTransformer

* some unit tests on ser-des options
@vkarpov15
Copy link
Contributor

Overall looks reasonable, just had a couple of suggestions/thoughts:

  1. I thought the plan was to make httpClient public on client and db?
  2. It would be great to improve error message when mixing up collections and tables. For example, if you try to insert a doc using table logic into a collection, you end up with the following error message:
    TypeError: Cannot convert undefined or null to object
      at Function.entries (<anonymous>)
      at TableSerDes.adaptDesCtx (node_modules/@datastax/astra-db-ts/dist/documents/tables/ser-des/ser-des.js:38:53)
      at TableSerDes.deserializeRecord (node_modules/@datastax/astra-db-ts/dist/lib/api/ser-des/ser-des.js:51:26)
      at CommandImpls.insertOne (node_modules/@datastax/astra-db-ts/dist/documents/commands/command-impls.js:50:38)

@toptobes
Copy link
Collaborator Author

toptobes commented Dec 22, 2024

I thought the plan was to make httpClient public on client and db?

DataAPIClient doesn't actually have an HttpClient, but it is public on Db, Table, Collection, and the three *Admin classes as ._httpClient.

The _ prefix remains to keep it out of autocomplete and to signify that it's really not a common property you should be accessing and working with.

Anyways, you shouldn't need the Feature-Flag hack anymore anyways since tables are enabled by default now (right?). But, if you did need any other Feature-Flag in the future, there's a new dbOptions.additionalHeaders options for specifying that sort of thing in the DataAPIClientOptions (or just additionalHeaders in DbOptions)

For example, if you try to insert a doc using table logic into a collection, you end up with the following error message...

Oh yeah I forgot about this case, I'll add some checks that'll basically say "hey you inserted a collection doc into a table and it succeeded but we failed to parse the response since you're using the table class", or, for finds, "hey you tried to find on a table but it was actually a collection so we failed".

Thanks for pointing this one out 👍

If you have any other thoughts/suggestions, no matter how nitpicky or small, please do share

* start work on more docs md pages

* created colls/tables datatype cheatsheet

* did collections dts

* readme work

* ran npm audit fix

* minor tweaks to DATATYPES.md

* bnunch of tsdoc for create-table related types

* tsdoc for DataAPILoggingDefaults

* update examples to use @next version of astra-db-ts

* documented list-tables
@vkarpov15
Copy link
Contributor

A couple of other issues I ran into:

  1. DataAPITimestamp's SerializeForCollection appears to serialize DataAPITimestamps as { $date: string } rather than { $date: number }, which leads to DataAPIResponseError: Bad JSON Extension value: Date ($date) needs to have NUMBER value, has STRING when trying to save a DataAPITimestamp into a collection.
  2. Why doesn't table serialization automatically handle converting JavaScript dates into timestamps?

@toptobes
Copy link
Collaborator Author

  1. Ope, thanks for pointing that out, definite bug
  2. It uses DataAPITimestamp for symmetry with the other dates/durations; also ensures people realise Datedate. That being said, I suppose I could still allow you to insert as Dates, but you'd still read them back as DataAPITimestamps. Also if you really want to use Dates instead of Timestamps for both r/w, there's a codec for that in TableCodecs, but it's undocumented atm since the ser/des options are still in beta

@vkarpov15
Copy link
Contributor

Re: DataAPITimestamp point (2), I would recommend automatically serializing JS dates to DataAPITimestamp because they're fundamentally the same type: integer containing milliseconds since epoch. Not much reason to make a distinction between the two. Is there even a reason to have a separate DataAPITimestamp type?

Right now sending a JS Date to insertOne() results in a "Error trying to convert to targetCQLType TIMESTAMP from value.class java.util.HashMap, value {}. Root cause: no codec matching value type." error, so I think the two options are (1) serialize Dates automatically, (2) throw a more readable error indicating that Dates are not serialized automatically. The current error is not great because users don't know why dates are getting serialized into hashmaps.

Some more notes:

  1. It doesn't look like I can import TableIndexDescriptor, and without that I can't use listIndexes() because of error Return type of public method from exported class has or is using name 'TableIndexDescriptor' from external module "node_modules/@datastax/astra-db-ts/dist/astra-db-ts" but cannot be named.
  2. There's a createIndex() method on the Table class, why is there no dropIndex()? Looks like there's just a dropTableIndex() on db, but that seems inconsistent.
  3. No BigInt support for tables? Looks like enableBigNumbers is only allowed on collections. Also, why is BigInt support behind a flag?

@toptobes
Copy link
Collaborator Author

toptobes commented Dec 29, 2024

Regarding #1 & #2, the main reason for the separate DataAPITimestamp is consistency; otherwise, there would be both Date and DataAPIDate classes, which may be a bit confusing/unintuitive.

I'm leaning towards throwing a readable error (agree that it's currently not readable) that says "either use a DataAPITimestamp, or enable the codec to use Dates instead".

I'm on the fence about serializing dates but still reading them back as a DataAPITimestamp, as it could be somewhat surprising behavior, but at the same time, we allow the same with vectors and such, so Idk 🤷

It doesn't look like I can import TableIndexDescriptor

Oops, bug, will fix in next preview release.

If you really want a quick fix for implementation/testing purposes, you can use

type TableIndexDescriptor = Awaited<ReturnType<InstanceType<typeof Table>['listIndexes']>> extends (infer T extends object)[] ? T : never;

There's a createIndex() method on the Table class, why is there no dropIndex()

This came from the Data API team themselves. On the Data API as well, dropIndex is a keyspace operation, while the create*Index operations are all table operations.

The idea is to make it clearer that index names exist within the keyspace, and not the table, so it reduces the chance of something dropping a keyspace that they didn't mean to.

This one's out of my control 🤷

No BigInt support for tables?

No, there is complete bigint (and BigNumber) support for tables; it's automatically enabled if you're using a table with varint or decimal columns (at the cost of some performance)

Looks like enableBigNumbers is only allowed on collections. Also, why is BigInt support behind a flag?

Actually, when someone uses bigints or BigNumbers, I need to fall back to the json-bigint json library, which is a drop-in replacement for the native JSON library, except it has actual bigint/BigNumber support.

However, this library is written purely in JS, unlike the native JSON module, which is typically implemented in highly-optimized C++, and is therefore decently slower. Plus, it sets a null prototype for each JS object, so I need to recursively fix the proto for each parsed object and its nested objects as well

TL;DR: it's a bit of a mess because the native JSON module doesn't support big numbers, and the Data API doesn't want to allow big numbers to be read as, or even written as, strings; instead, they're insistent on having bignumbers be raw JSON number literals, since it's technically allowed in the spec.

Can expand if necessary, but I can't really enable bignumbers by default for collections (I technically could for serialization, but not for deserialization; however, I'd rather have it explicitly on/off instead of having some half/half behavior).

I'll try to throw a readable error here as well for bignums on collections that don't have bignums enabled

@vkarpov15
Copy link
Contributor

Re: BigInts, I looked a little further into it, and here's where my misunderstanding is:

  1. BigInt deserialization is only enabled for decimal and varint, but not long. Ideally should also deserialize longs into bigints since max long is much larger than JavaScript's max safe integer
  2. No BigInt serialization for tables. json-bigint is only used for deserialization, but there's no code path that I see which uses json-bigint for serialization (tables or collections). That means the only way to serialize BigInts is to convert them to numbers first.

@toptobes
Copy link
Collaborator Author

toptobes commented Jan 2, 2025

BigInt deserialization is only enabled for decimal and varint, but not long.

oh you're right... I forgot about that case... I suppose bigint columns should always be using/returning bigints instead of numbers instead...

But there's no code path that I see which uses json-bigint for serialization (tables or collections)

No, there very much is. I can't say that the code for it is the cleanest since it's a pretty messy situation in the first place that had to be retrofitted since I didn't expect the Data API to go this route, but it exists.

This is in data-api-http-client.ts, in the executeCommand method:

const serialized = (info.bigNumsPresent)
  ? this.bigNumHack?.parser.stringify(info.command) // optional since it's not present in DataAPIDbAdmin usages. Not super happy with the loose invariant, but it is what it is
  : JSON.stringify(info.command);

Example:

> await table.insertOne({ text: '1', int: 1, varint: 1231231222132132131231231231231231231231232132133n })
{ insertedId: { text: '1', int: 1 } }
> await tfa
[
  {
    varint: 1231231222132132131231231231231231231231232132133n,
    int: 1,
    text: '1'
  }
]

@toptobes toptobes force-pushed the KG-v2.0 branch 2 times, most recently from 25a764d to 8ee74e2 Compare January 3, 2025 18:26
@vkarpov15
Copy link
Contributor

One more point I noticed:

  1. Do you intend to add returnDocumentResponses support for insertMany()? Looks like that is still hardcoded to returnDocumentResponses: true under the hood.

@toptobes
Copy link
Collaborator Author

toptobes commented Jan 7, 2025

Do you intend to add returnDocumentResponses support for insertMany()? Looks like that is still hardcoded to returnDocumentResponses: true under the hood.

As far as I'm aware, the implementation still doesn't work if the vectorize provider fails; still waiting on the Data API to fix that before it can be safely exposed for general use.

toptobes and others added 3 commits January 22, 2025 23:14
* UnexpectedDataAPIResponseError

* made column definition optional in tableserdesfns

* fixes serdes of datapitimestamp for collections

* move test script documentation to test script itself.

* separate scripts/check.sh file for typechecking/linting

* forJSEnv utility

* more work on blob & vector unit tests

* proper checking for Bignumbers

* killed DataAPITimestamp

* serializeGuard-based codecs now have higher priority

* bigints & counters now use bigint

* a lot more datatypes test work

* check for broken compilation when skipLibCheck: false w/ astra-db-ts as dep

* updated build script to have report updating be conditional

* reexport from version file

* more datatype tests

* enhanced coll bignumbers support

* split dates/times/durations into separate files

* overhaul of dates

* overhaul of times

* work for repl scirpt

* DataAPIDuration massive overhaul

* remoaved feature-flag tables header

* lots more tests and such
@vkarpov15
Copy link
Contributor

@toptobes can you please add back Feature-Flags-tables header in v2.0.0-preview.2? That seems to break all of my local tables tests, and there's no good way to set that header in v2.0.0-preview.2 from clients because forTableSlashCollectionOrWhateverWeWouldCallTheUnionOfTheseTypes doesn't support passing in header providers, only opts?.embeddingApiKey; and TypeScript puts the following code in the HttpClient constructor so there's no way to hack this header onto the baseHeaders prototype.

        Object.defineProperty(this, "baseHeaders", {
            enumerable: true,
            configurable: true,
            writable: true,
            value: void 0
        });

@vkarpov15
Copy link
Contributor

Also another potential issue I noticed: looks like includeSimilarity option for find() leads to Cannot read properties of undefined (reading 'type') error in tables. Data API response looks like the following - I suspect the issue is because Data API projectionSchema doesn't include an entry for $similarity.

{
  "data": {
    "documents": [
      {
        "name": "Test vector 1",
        "vector": [
          1,
          100
        ],
        "_id": "67954a1fdcee18121f6b60c2",
        "$similarity": 1
      },
      {
        "name": "Test vector 2",
        "vector": [
          100,
          1
        ],
        "_id": "67954a1fdcee18121f6b60c3",
        "$similarity": 0.51004946
      }
    ],
    "nextPageState": null
  },
  "status": {
    "projectionSchema": {
      "_id": {
        "type": "text"
      },
      "name": {
        "type": "text"
      },
      "vector": {
        "type": "vector",
        "dimension": 2
      }
    }
  }
}

@toptobes
Copy link
Collaborator Author

Regarding Feature-Flag-Tables, I was specifically requested to remove that header by the FE team, as it was causing them CORs errors

I think you should be able to get around it by setting additionalHeaders in DataAPIClientOptions.dbOptions

@toptobes
Copy link
Collaborator Author

I'll double check about includeSimjlarity, I think it should've been accidentally fixed in a local branch where I made some major serdes overhauls

* revamp of how codecs work

* cleanup of unused serdes stuff

* serdes code deduplication

* ctx.afterMap

* keytransformer overhaul

* colorized check scirpt

* internal serdes refactors + tests

* made table codec properly recursive

* serdes code deduplication/cleanup

* work on tests

* better detection of unit tests being skipped
@vkarpov15
Copy link
Contributor

I'm also getting the following error with enableBigNumbers: true on collections, because ctx.getNumRepForPath === true. Is this because enableBigNumbers needs to be a function now?

     TypeError: ctx.getNumRepForPath is not a function
      at coerceBigNumber (node_modules/@datastax/astra-db-ts/dist/documents/collections/ser-des/big-nums.js:85:17)
      at Array.deserialize (node_modules/@datastax/astra-db-ts/dist/documents/collections/ser-des/ser-des.js:50:51)
      at applySerdesFns (node_modules/@datastax/astra-db-ts/dist/lib/api/ser-des/ser-des.js:137:27)
      at deserializeRecord (node_modules/@datastax/astra-db-ts/dist/lib/api/ser-des/ser-des.js:105:18)
      at deserializeRecordHelper (node_modules/@datastax/astra-db-ts/dist/lib/api/ser-des/ser-des.js:131:9)
      at deserializeRecord (node_modules/@datastax/astra-db-ts/dist/lib/api/ser-des/ser-des.js:108:13)
      at CollectionSerDes.deserialize (node_modules/@datastax/astra-db-ts/dist/lib/api/ser-des/ser-des.js:46:16)
      at CommandImpls.findOne (node_modules/@datastax/astra-db-ts/dist/documents/commands/command-impls.js:195:29)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)
      at Context.<anonymous> (tests/driver/collections.driver.test.ts:224:28)

The Data API response from findOne() is the following:

{
  "data": {
    "document": {
      "_id": "679902eec2f47f1652180ef5",
      "name": "Very expensive product",
      "price": "9007199254740999"
    }
  }
}

toptobes and others added 4 commits January 28, 2025 22:18
* test.sh.md

* check to ensure that test suite names follow convention

* some stargate script updates + docs

* check.sh.md

* repl.sh.md

* logs only to stderr by default instead of split between stderr and stdout

* more dev docs updates

* set-example-client-deps.sh

* scripts/playgound.sh

* client?: "default" => client: "fetch-h2"

* migrate options parsing to 3rd party ts lib

* monoids

* tests passing again after more refactors (i love monoids)

* update dev deps

* majorly improve composability of options handlers

* split ctx.continue() into ctx.replace() and ctx.nevermind(); made deser codecs run backwards

* BaseSerDesCtx.target
* update eslint config

* run eslint --fix + fix other linting issues

* added & fixed consistent-type-* lints

* migrated source code to esm

* made tests run using tsx/esm import

* removed hard unstable dep on fetch-h2... now defaults to plain fetch by default

* removed hard unstable dep on events... now uses minimal event emitter implementaiton

* added dual support for cjs & esm

* use tslib

* update examples to work with latest version of astra-db-ts

* fixed node10 & node16:cjs type resolution issues

* update examples to reflect new compat changes

* update some http options documentation

* update readme
@vkarpov15
Copy link
Contributor

I ran into a few issues with 2.0.0-preview.4:

  1. TypeScript exports are all broken: if I do import { DataAPIVector } from '@datastax/astra-db-ts', I no longer can do if (v instanceof DataAPIVector) because DataAPIVector is exported with export type. The fix should be to just replace echo "export type * from '../astra-db-ts.d.ts';" > dist/esm/index.d.ts with echo "export * from '../astra-db-ts.d.ts';" > dist/esm/index.d.ts.
  2. I don't like the rename of CollectionSerDesConfig -> CollSerDesConfig. Inconsistent abbreviation usage leads to frustrating DX: either use Coll for everything or Collection for everything, unless there's some good reason
  3. Even with fix from (1), import { Collection } from '@datastax/astra-db-ts' gets you Collection = undefined. import { Table }, etc. seems fine. There's some weirdness where dist/cjs/documents/index.js has exports.Collection = undefined but dist/cjs/documents/collections/index.js has correct exports.Collection. Maybe some circular import issue or something? I haven't been able to figure this one out.

@toptobes
Copy link
Collaborator Author

toptobes commented Feb 6, 2025

ah crud, looks like I screwed up how I was validating that everything was still working (normally I use a symlink to dist so the playground project's dependency on astra-db-ts is reloaded every time, but I npm-installed a tar snapshot of the project and forgot about it, yadda yadda yadda, basically I was testing on an older build of astra-db-ts where it was still working)

I'll get a fix out for it ASAP

(also just using export * from '../astra-db-ts.d.ts' has its own issues since its a .d.ts file but I'll figure something out)

Regarding CollSerDesConfig, I mainly changed it to be as such since all of the internal types start with Coll for brevity, but I suppose you're right, the public API at the very least should remain Collection

@toptobes
Copy link
Collaborator Author

toptobes commented Feb 6, 2025

Tentatively fixed thanks to this stackoverflow answer, agree with the commenter that the behavior is honestly inintuitive

The index.d.ts files were changed to import * from '../astra-db-ts.js', where astra-db-ts.js is literally just an empty file in the root dist directory

toptobes and others added 2 commits February 6, 2025 23:21
* hotfix for broken types

* clean up & speed up build script

* change Coll => Collection prefixes as per vals suggestion

* check to make sure esm & cjs exports are the same

* build step to remove #private from rollup .d.ts
@vkarpov15
Copy link
Contributor

2.0.0-preview.5 seems to fix all issues, my tests now pass. Thanks!

@toptobes
Copy link
Collaborator Author

toptobes commented Feb 7, 2025

Can not begin to tell you how much of a relief that is. Thanks for verifying!

* made logging event emitters heirarchical

* add verbose stdout & stderr logging targets

* allow regex matching logging comand names

* some renaming + adding stop[Immediate]Propagation() to BaseClientEvent

* some logging bubblign/hierarchy tests

* update documentation for logging

* start adding logging examples

* more logging examples

* BaseClientEvent.requestId

* extended logging formatting + more logging docs

* extra log info for some commands

* readme for loggign examples

* made events deep clone dataapiresp object
src/db/db.ts Outdated
@@ -1156,6 +1164,7 @@ export class Db {
const resp = await this.#httpClient.executeCommand(command, {
timeoutManager: this.#httpClient.tm.single('tableAdminTimeoutMs', options),
keyspace: options?.keyspace,
extraLogInfo: options?.nameOnly ? 'name only' : 'with options',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of this logging pattern, I think it would be better to just log the options as-is. Otherwise you need to document and users need to understand how to interpret "name only" vs "with options"

Copy link
Collaborator Author

@toptobes toptobes Feb 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I didn't like it either. I can't just log the options either though due to how long they would be.

I have an update on a local branch which makes it look more structured like this instead:

2025-02-12 13:18:23 CDT [32a4fe9b] [CommandStarted]: test_coll::insertMany {records=1,ordered=false}
2025-02-12 13:18:24 CDT [32a4fe9b] [CommandFailed]: test_coll::insertMany {records=1,ordered=false} (865ms) ERROR: 'Document field name invalid: field name ('$invalid') contains invalid character(s), can contain only letters (a-z/A-Z), numbers (0-9), underscores (_), and hyphens (-)'

whereextraLogInfo is now an object instead of a string.

Must say, it is really tricky to balance the logs to be somewhat useful, but not overwhelmingly long lol

I've tried to make it clear that the standard stdout/stderr outputs are opinionated defaults for logging, and there are ways to implement your own logging system or override the log format if you really want to.

protected constructor(parent: Pick<HierarchicalEmitter<Events>, 'emit'> | null);
emit<E extends keyof Events>(eventName: E, event: Events[E]): void;
off<E extends keyof Events>(eventName: E, listener: (e: Events[E]) => void): void;
on<E extends keyof Events>(eventName: E, listener: (e: Events[E]) => void): () => void;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to prevent calling off, on, etc. if not emits: ['event']? It doesn't look like events are emitted if emits is set to a different value, but I might be wrong here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not statically, no (of course)

But I could, in theory, have a runtime check which errors if these methods were used if events: 'event' weren't set for a specific event.

But it wouldn't really make sense to, because:

  • It adds extra complexity which isn't really necessary IMO
  • Uou could call dataAPIClient.on() for an event which isn't enabled yet, but is enabled in a child class (e.g. a Db which was spawned from that client)
  • It closes the door for having a way to enable/disable events on an already created object in the future

public emit<E extends keyof Events>(eventName: E, event: Events[E]): void {
if (this.#listeners[eventName]) {
for (const listener of this.#listeners[eventName]) {
listener(event);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given error propagation, is it worth it to try/catch here? Right now any sync errors in the listener would prevent bubbling, which may or may not be intentional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good catch... will definitely do so

* update documenation to use Table.schema()

* fix bug in playground script

* allow async cursor.forEach() callbacks

* update to error obj v2 + some error docs

* fix broken tests

* work making logging outputs more structured/readable

* lil bit more work making logs less verbose/more relavant

* rip out keyTransformer

* rip out normalizedSort, replace with codec update

* added validation for codec classes

* require a default "*" for CollNumRepCfg

* catch errors thrown in listeners so they dont prevent bubbling

* lot of work documenting http stuff

* worlds shittiest way to enforce typescript dependency version

* remove now irrelevant part of readme

* some renamings and such

Object.entries(cfg).forEach(([path, rep]) => {
Object.entries(cfg).forEach(([path, coercion]) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor perf nitpick I've learned from Mongoose: path.split('.') is surprisingly slow when there's no ., better to check if there is a . first. Here's a quick JS benchmark:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants