refactor(data-types): convert data-types to TS #13799

wbourne0 · 2021-12-21T02:41:31Z

Pull Request Checklist

Please make sure to review and check all of these items:

Have you added new tests to prevent regressions?
Does npm run test or npm run test-DIALECT pass with this change (including linting)?
Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
Did you update the typescript typings accordingly (if applicable)?
Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description Of Change

converts data-types to TS.

I've typed this in a way that the types of columns can be extracted from sequelize.define (in theroy - haven't gotten far enough to test that yet). I also added some generic types to assist in getting types of a DataType.

For any reviewers, I've labled 3 (ts) types which are relevant to each DataType:

"Acceptable" type (ie what it'll accept as user input, e.g. for dates this is numbers, Dates, or strings)
"Sane" type - What sequelize will normally return (e.g. for dates, Date)
"Raw" type - Whatever we need to deserealize for a type. (e.g. for a boolean this is a number, buffer, or string)

Child PRs

The approach of this PR

Due to the size of this change, I've opted to split up this PR into many to make it more reviewable.

Here's the steps I'll be taking:

Create a "master" pr containing the changes which other data-types prs will be based on (this pr)
For each dialect, create a PR based on this branch containing the needed changes.
Once this PR is approved, merge in the child prs as they are approved.
Once all of the dialect specific code is in (meaning: all lib/ code is approved and data-types should be working), start converting the tests to TS (in distinct prs, again for reviewability).
Once all test prs are approved & merged, merge this pr (assuming tests are green)

WikiRik · 2021-12-21T06:57:32Z

@allawesome497 checkout #12941 which was updated this week

wbourne0 · 2021-12-21T07:56:35Z

@allawesome497 checkout #12941 which was updated this week

Oh I completely missed that.

My change covers a few things that aren't handled in that pr such as named exports, inline static keys (e.g. key is then declared in the class itself rather than being exposed later), and more specific typing for types which vary from their raw values (in particular: "sane" vs "accepted" vs "raw" as I've called them).

sdepold · 2021-12-21T09:15:27Z

@jesse23 @Keimeno Up for a review?

sdepold · 2021-12-21T09:16:56Z

I think my TS skills don't really suffice to review this beast. I have an opinion the wording however:

"Acceptable" type (ie what it'll accept as user input, e.g. for dates this is numbers, Dates, or strings)
--> InputType

"Sane" type - What sequelize will normally return (e.g. for dates, Date)
--> SanitizedType

"Raw" type - Whatever we need to deserealize for a type. (e.g. for a boolean this is a number, buffer, or string)
--> OutputType

sdepold · 2021-12-21T10:27:55Z

Adding @ephys as well :)

jesse23 · 2021-12-21T10:35:22Z

@allawesome497 2 questions in general...

It is possible enable the test 1stly, then try to do pair converting with the test? For example in this case, no need to convert all impacted test but at least the major test/unit/sql/data-types.test.js.
Shall we merge with data-types.d.ts ( and eliminate data-types.d.ts eventually )?

ephys

Looks good to me, this is my only note

ephys · 2021-12-23T14:58:49Z

lib/utils/class-to-invokable.ts

-      return new Class(...args);
-    },
+export function classToInvokable<
+  Class extends new (...args: Array<never>) => any


is it normal that args is Array<never>? Shouldn't it be Array<any> or Array<unknown>?

This could also be Array<any> (and that's what I'd normally put). I just did this since eslint recommended it and I was unsure if it'd work. Will probably change it back to Array<any> since that's more intuitive IMO.

wbourne0 · 2021-12-25T05:50:54Z

I think my TS skills don't really suffice to review this beast. I have an opinion the wording however:

"Acceptable" type (ie what it'll accept as user input, e.g. for dates this is numbers, Dates, or strings) --> InputType

"Sane" type - What sequelize will normally return (e.g. for dates, Date) --> SanitizedType

"Raw" type - Whatever we need to deserealize for a type. (e.g. for a boolean this is a number, buffer, or string) --> OutputType

Sounds good, though I think that RawType is a bit clearer than OutputType since its more of an "internal" output type,

Boolean is one of the types where this is more relevant, since a "raw" boolean can be a buffer, string, or number imo.

sdepold · 2022-01-09T12:41:55Z

How are we doing on this front? I just wanted to point out one more time, that we are now targeting v7 and could hence drop support for any weird hack that was introduced earlier.

wbourne0 · 2022-01-19T05:55:27Z

How are we doing on this front? I just wanted to point out one more time, that we are now targeting v7 and could hence drop support for any weird hack that was introduced earlier.

Have been pretty busy recently, back on it for now.

Looks like we'll need to re-model how DataTypes works but I don't think it'll be too bad (ie most standard usage should be fine iiuc).

ephys · 2022-01-19T08:04:12Z

I'm researching how we could support customising what type Sequelize must output for a given DataType. I'm waiting on this PR to be merged before properly experimenting but since you've knee-deep in DataTypes I'd like to know if my current draft would impact your decisions in this PR, like for SaneTypeOf https://gist.github.com/ephys/50a6a05be7322c64645be716fea6186a#support-for-alternative-js-types

Also I'd still like to bikeshed the names of the 3 types based on how I understood them:

"Acceptable" -> UserInputTypeOf
"Sane" SequelizeOutputTypeOf
"Raw" -> SqlInputTypeOf

wbourne0 · 2022-01-21T05:20:24Z

I'm researching how we could support customising what type Sequelize must output for a given DataType. I'm waiting on this PR to be merged before properly experimenting but since you've knee-deep in DataTypes I'd like to know if my current draft would impact your decisions in this PR, like for SaneTypeOf https://gist.github.com/ephys/50a6a05be7322c64645be716fea6186a#support-for-alternative-js-types

Also I'd still like to bikeshed the names of the 3 types based on how I understood them:
* "Acceptable" -> UserInputTypeOf

* "Sane" SequelizeOutputTypeOf

* "Raw" -> SqlInputTypeOf

Nope, I think your current draft is fine - though it will be a pain to migrate typing to this.

As I'm finding, there's a lot of undocumented dialect-specific logic around typing :/.

As for renaming: I will do this, I just haven't yet since there hasn't been a clear choice in terms of names yet (and I don't see this PR getting merged soon, given its scale so I figure its worth waiting).

ephys · 2022-01-21T08:45:52Z

though it will be a pain to migrate typing to this.

Yup! Not looking forward to it but I want the feature enough to be motivated at least

wbourne0 · 2022-02-10T03:01:41Z

@allawesome497 2 questions in general...

🤔 I thought I responded to this but I don't see any response.

It is possible enable the test 1stly, then try to do pair converting with the test? For example in this case, no need to convert all impacted test but at least the major test/unit/sql/data-types.test.js.

I've opted to convert everything to TS first and add tests later (however: this will be before this PR is merged). I added some more details to the PR description.

Shall we merge with data-types.d.ts ( and eliminate data-types.d.ts eventually )?

Yep! That should be done by the time I merge this if I haven't already removed the file.

wbourne0 · 2022-03-05T02:12:43Z

For renaming, thoughts on:

AcceptedType -> InputType
SaneType -> OutputType
RawType -> ParsedType

CC @sdepold @ephys

converts data-types to TS

ephys · 2022-03-22T10:06:34Z

src/dialects/abstract/data-types.ts

+      };
+    }
+
+    this.options = options;


I think it would be a good opportunity to normalize options. We should either have options, or _length & co. But not both, otherwise we need to keep both in sync and it's error prone.

ephys · 2022-03-22T10:22:43Z

src/dialects/abstract/data-types.ts

+  constructor(precision: number, scale?: number);
+  constructor(...args: DecimalConstructorParams);
+  constructor(precision?: number | DecimalOptions, scale?: number) {
+    if (typeof precision === 'object') {


use isObject from lodash instead

ephys · 2022-03-22T10:23:21Z

src/dialects/abstract/data-types.ts

+    }
+
+    this._precision = this.options.precision;
+    this._scale = this.options.scale;


same comment about deduplicating options & instance fields

ephys · 2022-03-22T10:24:42Z

src/dialects/abstract/data-types.ts

+  public validate(value: AcceptedNumber): asserts value is AcceptedNumber;
+  public validate(value: AcceptedNumber): true {


can't this be simplified to:

Suggested change

public validate(value: AcceptedNumber): asserts value is AcceptedNumber;

public validate(value: AcceptedNumber): true {

public validate(value: AcceptedNumber): asserts value is AcceptedNumber {

ephys · 2022-03-22T10:25:29Z

src/dialects/abstract/data-types.ts

+    return 'DECIMAL';
+  }
+
+  public validate(value: AcceptedNumber): asserts value is AcceptedNumber;


AcceptedNumber doesn't include string. But DECIMAL should accept strings to avoid loss of precision

ephys · 2022-03-22T10:56:56Z

src/dialects/abstract/data-types.ts

+  RawType = AcceptedType,
+> {
+  public static readonly key: string = 'ABSTRACT';
+  // @internal


doesn't stripInternal only apply to jsdoc? (either way this should be jsdoc)

ephys · 2022-03-22T10:57:14Z

src/dialects/abstract/data-types.ts

+  ? Instance
+  : T;
+
+export type AcceptableTypeOf<T extends DataType> =


the purpose of these different types should be documented

ephys · 2022-03-22T10:58:21Z

src/dialects/abstract/data-types.ts

+
+// If T is a constructor, returns the type of what `new T()` would return-
+// otherwise, returns T
+export type Constructed<T> = T extends abstract new () => infer Instance


isn't this the same as the built-in type InstanceType?

ephys · 2022-03-22T10:58:44Z

src/dialects/abstract/data-types.ts

+import { logger } from '../../utils/logger';
+import { validator as Validator } from '../../utils/validator-extras';
+
+const warnings: Record<string, boolean> = {};


Suggested change

const warnings: Record<string, boolean> = {};

const warnings: Record<string, boolean> = Object.create(null);

ephys · 2022-03-22T10:59:44Z

src/data-types.ts

@@ -0,0 +1,9 @@
+export * from './dialects/abstract/data-types';
+export * as db2 from './dialects/db2/data-types';


what is the reason for exposing these? Shouldn't users only use DataTypes.x and not DataTypes.postgres.x?

ephys · 2022-03-22T11:00:54Z

Some comments may be unrelated to this PR.

AcceptedType -> InputType
SaneType -> OutputType
RawType -> ParsedType

As long as they're well documented, I'm fine with it

ephys · 2022-03-22T11:05:41Z

src/dialects/abstract/data-types.ts

+      };
+
+      Reflect.defineProperty(this, 'types', prop);
+      Reflect.defineProperty(this.prototype, 'types', prop);


Is there a reason to add it to the prototype too?

ephys · 2022-03-22T11:11:01Z

src/dialects/abstract/data-types.ts

+    return null;
+  }
+
+  static readonly parse = this.prototype._sanitize;


I'd prefer to have the _sanitize instance method call the static parse method instead of getting something from the prototype

ephys · 2022-03-22T11:14:09Z

src/dialects/abstract/data-types.ts

+
+  public toSql() {
+    if (this._precision || this._scale) {
+      return `DECIMAL(${[this._precision, this._scale]


if scale is specified but precision is not, we should throw with a clear error message

ephys · 2022-03-22T11:17:05Z

src/dialects/abstract/data-types.ts

+      case 'tiny':
+        return 'TINY_TEXT';
+      case 'medium':
+        return 'MEDIUM_TEXT';
+      case 'long':
+        return 'LONG_TEXT';
+      default:
+        return this.key;


Why were these changed, they should be TINYTEXT & co

ephys · 2022-03-22T11:18:07Z

src/dialects/abstract/data-types.ts

+      );
+    }
+
+    return true;


We should remove these return true from validate.

wbourne0 · 2022-09-16T18:59:41Z

Closing in favor of #14505.

wbourne0 added type: typescript For issues and PRs. Things that involve typescript, such as typings and intellisense. status: wip For issues and PRs. Applied when the PR is not ready yet / when work to close the issue has started. labels Dec 21, 2021

wbourne0 self-assigned this Dec 21, 2021

ephys reviewed Dec 23, 2021

View reviewed changes

sdepold mentioned this pull request Jan 10, 2022

How do we approach the TypeScript conversion? sequelize/meetings#6

Open

ephys mentioned this pull request Jan 13, 2022

fix(types): fix QueryInterface#bulkInsert attribute arg type #13945

Merged

6 tasks

wbourne0 added type: refactor For issues and PRs. Things that improve the code readability, maintainability, testability, etc. hard For issues and PRs. labels Jan 19, 2022

wbourne0 mentioned this pull request Jan 19, 2022

refactor(data-types): convert postgres data-types to TS #13970

Closed

wbourne0 mentioned this pull request Jan 21, 2022

refactor(data-types): convert mysql data-types to TS #13981

Closed

github-actions bot added the stale label Feb 5, 2022

ephys removed the stale label Feb 6, 2022

wbourne0 mentioned this pull request Feb 10, 2022

refactor(data-types): convert snowflake data-types to TS #14081

Closed

This was referenced Mar 10, 2022

refactor(data-types): convert db2 data-types to TS #14220

Closed

refactor(data-types): convert mssql data-types to TS #14238

Closed

refactor(data-types): convert sqlite data-types to TS #14239

Closed

wbourne0 mentioned this pull request Mar 12, 2022

refactor(data-types): convert mariadb data-types to TS #14240

Closed

refactor(data-types): convert data-types to TS

210279c

converts data-types to TS

wbourne0 force-pushed the typeify-data-types branch from 6de690b to 210279c Compare March 12, 2022 05:20

ephys mentioned this pull request Mar 17, 2022

refactor: migrate deferrable.js to ts #14252

Merged

6 tasks

ephys added a commit that referenced this pull request Mar 17, 2022

refactor: replace the classToInvokeable implem with #13799

5b9d5c3

ephys mentioned this pull request Mar 18, 2022

test: add unit tests for every datatype #14258

Merged

6 tasks

ephys requested changes Mar 22, 2022

View reviewed changes

Merge branch 'main' into typeify-data-types

bf78103

WikiRik mentioned this pull request Apr 21, 2022

feat: time with timezone #14213

Closed

6 tasks

Merge branch 'main' into typeify-data-types

6e0eb40

ephys mentioned this pull request May 13, 2022

feat: rewrite Data Types #14505

Merged

42 tasks

wbourne0 closed this Sep 16, 2022

ephys deleted the typeify-data-types branch October 3, 2022 16:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(data-types): convert data-types to TS #13799

refactor(data-types): convert data-types to TS #13799

wbourne0 commented Dec 21, 2021 •

edited

Loading

WikiRik commented Dec 21, 2021

wbourne0 commented Dec 21, 2021

sdepold commented Dec 21, 2021

sdepold commented Dec 21, 2021

sdepold commented Dec 21, 2021

jesse23 commented Dec 21, 2021

ephys left a comment

ephys Dec 23, 2021

wbourne0 Dec 25, 2021

wbourne0 commented Dec 25, 2021

sdepold commented Jan 9, 2022

wbourne0 commented Jan 19, 2022

ephys commented Jan 19, 2022

wbourne0 commented Jan 21, 2022

ephys commented Jan 21, 2022

wbourne0 commented Feb 10, 2022 •

edited

Loading

wbourne0 commented Mar 5, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys commented Mar 22, 2022 •

edited

Loading

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

ephys Mar 22, 2022

wbourne0 commented Sep 16, 2022

		public validate(value: AcceptedNumber): asserts value is AcceptedNumber;
		public validate(value: AcceptedNumber): true {

	const warnings: Record<string, boolean> = {};
	const warnings: Record<string, boolean> = Object.create(null);

		@@ -0,0 +1,9 @@
		export * from './dialects/abstract/data-types';
		export * as db2 from './dialects/db2/data-types';

refactor(data-types): convert data-types to TS #13799

refactor(data-types): convert data-types to TS #13799

Conversation

wbourne0 commented Dec 21, 2021 • edited Loading

Pull Request Checklist

Description Of Change

Child PRs

The approach of this PR

WikiRik commented Dec 21, 2021

wbourne0 commented Dec 21, 2021

sdepold commented Dec 21, 2021

sdepold commented Dec 21, 2021

sdepold commented Dec 21, 2021

jesse23 commented Dec 21, 2021

ephys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbourne0 commented Dec 25, 2021

sdepold commented Jan 9, 2022

wbourne0 commented Jan 19, 2022

ephys commented Jan 19, 2022

wbourne0 commented Jan 21, 2022

ephys commented Jan 21, 2022

wbourne0 commented Feb 10, 2022 • edited Loading

wbourne0 commented Mar 5, 2022

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

ephys commented Mar 22, 2022 • 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

wbourne0 commented Sep 16, 2022

wbourne0 commented Dec 21, 2021 •

edited

Loading

wbourne0 commented Feb 10, 2022 •

edited

Loading

ephys commented Mar 22, 2022 •

edited

Loading