NODE-2717: Improve TS Typings #389

nbbeeken · 2020-09-03T22:03:58Z

Improved the typings, a few any cases had to be left in to favor leaving code more or less untouched rather than refactoring.

nbbeeken · 2020-09-03T22:10:51Z

src/constants.ts

- *
- * @classconstant BSON_BINARY_SUBTYPE_MD5
- **/
-export const BSON_BINARY_SUBTYPE_MD5 = 4;


We don't use this anywhere internally, but we do export it.
I added tests to make sure we have the correct values, who knows typos happen best to guard against that.

nbbeeken · 2020-09-03T22:11:48Z

src/extended_json.ts

+export function isBSONType(value: unknown): value is BSONType {
+  return (
+    isObjectLike(value) && Reflect.has(value, '_bsontype') && typeof value._bsontype === 'string'
+  );
+}


This makes for some very nice TS

if (isBSONType(x)) { x._bsontype // union of all the type names if(x._bsontype === 'Double') { x.toExtendedJSON() // function exists and return type can be narrowed to `{$numberDouble: string}` } }

src/parser/deserializer.ts

src/parser/serializer.ts

nbbeeken · 2020-09-03T22:25:46Z

src/timestamp.ts

+type TimestampOverrides = '_bsontype' | 'toExtendedJSON' | 'fromExtendedJSON';
+type LongWithoutOverrides = new (low: number | Long, high?: number, unsigned?: boolean) => {
+  [P in Exclude<keyof Long, TimestampOverrides>]: Long[P];
+};
+const LongWithoutOverridesClass: LongWithoutOverrides = (Long as unknown) as LongWithoutOverrides;

+export class Timestamp extends LongWithoutOverridesClass {
+  _bsontype!: 'Timestamp';


I know what you're thinking.. this looks gross! 😅
This is the only case where we extend from another BSON type but this means we can't just have different types for _bsontype and from/toExtendedJSON so this does some trickery to create a Long type that doesn't have those properties. The generated code is not effected it still extends Long as normal.

Another way to accomplish this would be to make something like:

class LongBase { ... the vendored Long implementation ... } export class Long extends LongBase { _bsontype!: 'Long'; fromExtendedJSON(..) {} toExtendedJSON(..) {} } export class Timestamp extends LongBase { _bsontype!: 'Timestamp'; fromExtendedJSON(..) {} toExtendedJSON(..) {} }

This approach might make it easier to use BigInt in the future as well?

This is a good solution!
But there's a type issues that arise in a few places, namely Long.isLong asserts that something is LongBase and then we would need a further check to see if something is Long with _bsontype
My solution lives only in TS land and doesn't effect code output. I know its a hack kinda but it feels like the right way to go to keep types consistent.

looks like the definitely typed typings use a similar approach to what I suggested?

~~Oh yea I could paramertize the Long class, I'll look into that.~~
Actually to make this work you have to duplicate the static methods on each class, since static things don't have access to the class type parameters. It's easy in the ambient context to just duplicate the function headers but in real code that mean actually copy pasting a bunch of bitwise shifting code on to two classes 😅

reggi · 2020-09-09T17:18:56Z

src/max_key.ts

-   * @ignore
-   */
-  toExtendedJSON() {
+  _bsontype!: 'MaxKey';


Is this necessary? If so shouldn't it be { value: ... }?

I noticed this around a lot, would love to learn more about the difference between this and the prototype addition to the class. Could just be _bsontype = { value: "MaxKey" } curious why not.

We still want it to be a string so we would want it to be _bsontype = "MaxKey" but defining it this way makes it an editable property on the MaxKey instance and not on its prototype. Also by defining it with a call to Object.defineProperty by default the property descriptor is

{ value: 'MaxKey', writable: false, enumerable: false, configurable: false }

So it is neither writable, enumerable, nor configurable. In addition to this, and I could be mistaken but the prototype is not editable outside the module so you can't change this once its exported which is good for security.

mbroadst · 2020-09-04T13:08:39Z

src/bson.ts

@@ -85,13 +88,12 @@ export const EJSON = {
  deserialize: EJSON_deserialize
 };

-export interface Document {
+export interface BSONDocument {


What's motivating this change?

Since there are DOM types here Document is the type of the global document object. We can rename this in the driver import but it seems best that this library doesn't clash.

src/bson.ts

mbroadst · 2020-09-10T13:00:40Z

src/code.ts

- * A class representation of the BSON Code type.
- */
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export type CodeFunction = (...args: any[]) => any;


Can we just use Function instead?

Is there something confusing about this type?
We can Function just throws warnings which I can silence, maybe even globally in the config but if not any funciton that works with this type will have to be annotated. This accepts any function.

Hm, I don't know where my comment to this went.. I'm wondering because in spirit the Code type is meant to be a wrapper around a JavaScript function, so it seemed weird to me that we would introduce a new type which accepts any function rather than just using the primitive type.

Maybe we can disable the Function rule just for the files where we need to use it? When it comes to BSON there will have to be exceptions, we already disable linter rules for any in a number of key places because.. well you need to support any sometimes! I'd rather not introduce new types just to get around a linter warning, if it's going to make the code less readable

src/code.ts

src/constants.ts

mbroadst · 2020-09-10T15:22:54Z

src/objectid.ts

   */
  toString(format?: BufferEncoding): string {
    // Is the id a buffer then use the buffer toString method to return the format
-    if (this.id && 'copy' in (this.id as any)) {
+    if (this.id && typeof this.id !== 'string' && 'copy' in (this.id as Buffer)) {


why the added check for string?

mbroadst · 2020-09-10T15:27:01Z

src/objectid.ts


    // The most common use case (blank id, new objectId instance)
    if (id == null || typeof id === 'number') {
      // Generate a new id
-      this.id = ObjectId.generate(id);
+      this.id = ObjectId.generate(typeof id === 'number' ? id : undefined);


now I see where all the checks for string are coming from. Let's solve this once and for all, the type for id should just be a Buffer

This was lost as well, we can do it in follow-on work but please make a ticket for that

src/parser/serializer.ts

src/validate_utf8.ts

mbroadst · 2020-09-10T15:35:23Z

src/timestamp.ts

+type TimestampOverrides = '_bsontype' | 'toExtendedJSON' | 'fromExtendedJSON';
+type LongWithoutOverrides = new (low: number | Long, high?: number, unsigned?: boolean) => {
+  [P in Exclude<keyof Long, TimestampOverrides>]: Long[P];
+};
+const LongWithoutOverridesClass: LongWithoutOverrides = (Long as unknown) as LongWithoutOverrides;

+export class Timestamp extends LongWithoutOverridesClass {
+  _bsontype!: 'Timestamp';


Another way to accomplish this would be to make something like:

class LongBase { ... the vendored Long implementation ... } export class Long extends LongBase { _bsontype!: 'Long'; fromExtendedJSON(..) {} toExtendedJSON(..) {} } export class Timestamp extends LongBase { _bsontype!: 'Timestamp'; fromExtendedJSON(..) {} toExtendedJSON(..) {} }

This approach might make it easier to use BigInt in the future as well?

nbbeeken

My orignal comments got lost in a browser refresh, please bare with the terseness of these as I tried to quickly make up for lost time.

src/parser/deserializer.ts

src/db_ref.ts

nbbeeken · 2020-09-10T19:16:43Z

src/extended_json.ts

+  );
+}
+
+export type EJSONDocument = { [key: string]: ReturnType<BSONType['toExtendedJSON']> };


Yes! unfortunately it also includes the relaxed return types as well, I wish there was a way to seperate them but this is a good start!

src/extended_json.ts

nbbeeken · 2020-09-10T19:20:18Z

src/extended_json.ts

-  Double: o => new Double(o.value),
-  Int32: o => new Int32(o.value),
-  Long: o =>
+  Binary: (o: {


Binary has sub_type and this accepts subtype, they're not compatible types.

nbbeeken · 2020-09-10T19:22:53Z

src/timestamp.ts

+type TimestampOverrides = '_bsontype' | 'toExtendedJSON' | 'fromExtendedJSON';
+type LongWithoutOverrides = new (low: number | Long, high?: number, unsigned?: boolean) => {
+  [P in Exclude<keyof Long, TimestampOverrides>]: Long[P];
+};
+const LongWithoutOverridesClass: LongWithoutOverrides = (Long as unknown) as LongWithoutOverrides;

+export class Timestamp extends LongWithoutOverridesClass {
+  _bsontype!: 'Timestamp';


This is a good solution!
But there's a type issues that arise in a few places, namely Long.isLong asserts that something is LongBase and then we would need a further check to see if something is Long with _bsontype
My solution lives only in TS land and doesn't effect code output. I know its a hack kinda but it feels like the right way to go to keep types consistent.

src/validate_utf8.ts

src/bson.ts

mbroadst · 2020-09-11T19:59:02Z

src/db_ref.ts

  collection: string;
  oid: ObjectId;
-  db: string;
-  fields: any;
+  db?: string | null;


did you make this change? It looks the same here, should just be db?: string right?

src/db_ref.ts

mbroadst · 2020-09-11T20:09:38Z

src/extended_json.ts

-  Double: o => new Double(o.value),
-  Int32: o => new Int32(o.value),
-  Long: o =>
+  Binary: (o: {


oof, that's unintentional I'm sure and probably a bug (it was never called subtype, even in 0.2.x)! My comment generally applies to this whole section though

src/parser/deserializer.ts

mbroadst · 2020-09-11T20:15:39Z

src/timestamp.ts

+type TimestampOverrides = '_bsontype' | 'toExtendedJSON' | 'fromExtendedJSON';
+type LongWithoutOverrides = new (low: number | Long, high?: number, unsigned?: boolean) => {
+  [P in Exclude<keyof Long, TimestampOverrides>]: Long[P];
+};
+const LongWithoutOverridesClass: LongWithoutOverrides = (Long as unknown) as LongWithoutOverrides;

+export class Timestamp extends LongWithoutOverridesClass {
+  _bsontype!: 'Timestamp';


looks like the definitely typed typings use a similar approach to what I suggested?

mbroadst · 2020-09-11T20:20:04Z

src/objectid.ts

@@ -42,10 +33,17 @@ function makeObjectIdError(invalidString, index) {
  );
 }

+export interface ObjectIdLike {


looks like this comment was lost in the page refresh

mbroadst · 2020-09-11T20:20:36Z

src/objectid.ts


    // The most common use case (blank id, new objectId instance)
    if (id == null || typeof id === 'number') {
      // Generate a new id
-      this.id = ObjectId.generate(id);
+      this.id = ObjectId.generate(typeof id === 'number' ? id : undefined);


This was lost as well, we can do it in follow-on work but please make a ticket for that

Simpler naming

mbroadst

LGTM! I left one comment about CodeFunction that I feel like 4/10 about, feel free to just merge if you strongly disagree with my position.

mbroadst · 2020-09-15T11:08:35Z

src/code.ts

- * A class representation of the BSON Code type.
- */
+// eslint-disable-next-line @typescript-eslint/no-explicit-any
+export type CodeFunction = (...args: any[]) => any;


Hm, I don't know where my comment to this went.. I'm wondering because in spirit the Code type is meant to be a wrapper around a JavaScript function, so it seemed weird to me that we would introduce a new type which accepts any function rather than just using the primitive type.

Maybe we can disable the Function rule just for the files where we need to use it? When it comes to BSON there will have to be exceptions, we already disable linter rules for any in a number of key places because.. well you need to support any sometimes! I'd rather not introduce new types just to get around a linter warning, if it's going to make the code less readable

nbbeeken added 2 commits September 2, 2020 17:22

WIP: correct linting errors

5a621c1

chore: resolve all linter warnings

4636828

nbbeeken requested review from reggi, emadum and mbroadst September 3, 2020 22:03

nbbeeken commented Sep 3, 2020

View reviewed changes

nbbeeken added 2 commits September 8, 2020 10:20

fix: remove dom lib and fix buffer write calls

ebac0e6

fix: toString toJSON methods on Binary

4a28e52

reggi reviewed Sep 9, 2020

View reviewed changes

reggi approved these changes Sep 9, 2020

View reviewed changes

mbroadst suggested changes Sep 10, 2020

View reviewed changes

nbbeeken commented Sep 10, 2020

View reviewed changes

Address comments

c27cc5f

nbbeeken requested a review from mbroadst September 11, 2020 14:30

fix: EJSON API accepts primitives

bc989fa

emadum approved these changes Sep 11, 2020

View reviewed changes

mbroadst suggested changes Sep 11, 2020

View reviewed changes

nbbeeken added 2 commits September 14, 2020 13:08

remove null from db prop on DbRef

2d437b1

fix: 🎨 Just use Document instead of BSONDocument

1d79e5a

Simpler naming

mbroadst approved these changes Sep 15, 2020

View reviewed changes

fix: 🔥 Remove custom CodeFunction type

4576fab

nbbeeken merged commit ae9ae2d into master Sep 15, 2020

nbbeeken deleted the NODE-2717/improve-ts branch September 15, 2020 14:50

github-actions bot mentioned this pull request May 7, 2024

chore(main): release 7.0.0 [skip-ci] #687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NODE-2717: Improve TS Typings #389

NODE-2717: Improve TS Typings #389

nbbeeken commented Sep 3, 2020

nbbeeken Sep 3, 2020

nbbeeken Sep 3, 2020

nbbeeken Sep 3, 2020

mbroadst Sep 10, 2020

nbbeeken Sep 10, 2020

mbroadst Sep 11, 2020

nbbeeken Sep 11, 2020 •

edited

Loading

reggi Sep 9, 2020

reggi Sep 9, 2020

nbbeeken Sep 9, 2020

mbroadst Sep 4, 2020

nbbeeken Sep 10, 2020

mbroadst Sep 10, 2020

nbbeeken Sep 10, 2020

mbroadst Sep 15, 2020

mbroadst Sep 10, 2020

mbroadst Sep 10, 2020

mbroadst Sep 11, 2020

nbbeeken Sep 11, 2020

mbroadst Sep 10, 2020

nbbeeken left a comment

nbbeeken Sep 10, 2020

nbbeeken Sep 10, 2020

nbbeeken Sep 10, 2020

mbroadst Sep 11, 2020

mbroadst Sep 11, 2020

mbroadst Sep 11, 2020

mbroadst Sep 11, 2020

mbroadst Sep 11, 2020

mbroadst left a comment

mbroadst Sep 15, 2020

NODE-2717: Improve TS Typings #389

NODE-2717: Improve TS Typings #389

Conversation

nbbeeken commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbroadst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbbeeken Sep 11, 2020 •

edited

Loading