TypeScript migration

[kellnerd]

Problem

Our ORM and its utility functions are still mostly plain JavaScript which makes it hard to use its deeply nested methods without having the definitions open side by side with the current code.

I had already started to sprinkle in a few minimal type definitions in #304 but going forward we realized that there are too many variations of how a model's data can look like (missing and additional properties for different usage scenarios) and that my initial naming schema was not sufficient to cover these.

Solution

• Update our own fork of bookshelf to also ship type definitions (which had to be adapted as well) and use it
• Introduce separate types for lazy-loaded properties and use consistent names for inserted and fetched models (breaking change, but for types only)
• Migrate all remaining JS files to TS and add types where possible: Utility functions have a pretty good type coverage while the way the Bookshelf models are defined makes it hard/impossible to add many types
• Move types for parsed entities over from the importer project and adapt them to the new naming conventions
• Standardize and deduplicate types which were already defined for ORM utility functions
• Fix errors in the existing code which were revealed by the new typings

The changes also include a few TODO items where our code should potentially be refactored to avoid confusing types and/or wrong usage in the future.
(Of course eslint is complaining about these...)

Pending Tasks

• Check that the remaining type errors have no actual influence and try to resolve them (b699ab0 was one case where an alternative Bookshelf usage pattern helped)
• Update code in bb-site which uses types that are still following the old naming convention (should be limited to BB-530: Show Wikipedia extracts for entities bookbrainz-site#943)
• Finally we can start to make use of the new exports (e.g. 3134f41) in bb-site

[kellnerd]

We currently have 13 6 type errors remaining:

• 7 errors are most likely caused by incomplete/incompatible Bookshelf type definitions (resolved)
• 4 errors are caused by date properties being of unknown type because of

  bookbrainz-data-js/src/func/entity.ts

  Lines 35 to 36 in b699ab0

  	// TODO: `entityData` should have the type ParsedEntity later which is currently causing lots of type issues.
  	entityData: Record<string, unknown>, entityType: EntityTypeString

• 2 errors are hinting at real issues with the entity set utilities:
  These are supposedly adapted from bb-site (I haven't found their equivalent yet, the closest thing is this), hence are not used in production and a possible reason why the import process fails with SQL errors

src/func/entity-sets.ts:59:32 - error TS2339: Property 'model' does not exist on type 'EntitySetMetadataT'.

59   orm, transacting, derivedSet.model, [...unchangedItems, ...addedItems],
                                  ~~~~~

src/func/entity-sets.ts:83:41 - error TS2339: Property 'model' does not exist on type 'EntitySetMetadataT'.

83   const oldSetRecord = await derivedSet.model.forge({
                                           ~~~~~

src/func/entity.ts:43:56 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.

43    const [beginYear, beginMonth, beginDay] = parseDate(beginDate);
                                                          ~~~~~~~~~

src/func/entity.ts:44:50 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.

44    const [endYear, endMonth, endDay] = parseDate(endDate);
                                                    ~~~~~~~

src/func/entity.ts:61:56 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.

61    const [beginYear, beginMonth, beginDay] = parseDate(beginDate);
                                                          ~~~~~~~~~

src/func/entity.ts:62:50 - error TS2345: Argument of type 'unknown' is not assignable to parameter of type 'string'.

62    const [endYear, endMonth, endDay] = parseDate(endDate);
                                                    ~~~~~~~

src/models/data/publisherData.ts:40:19 - error TS2339: Property 'query' does not exist on type 'typeof Model'.

40    return Edition.query((qb) => {
                     ~~~~~

src/models/revisions/authorRevision.ts:53:17 - error TS2351: This expression is not constructable.
  Type 'Function' has no construct signatures.

53      return new AuthorRevision()
                   ~~~~~~~~~~~~~~

src/models/revisions/editionGroupRevision.ts:53:17 - error TS2351: This expression is not constructable.
  Type 'Function' has no construct signatures.

53      return new EditionGroupRevision()
                   ~~~~~~~~~~~~~~~~~~~~

src/models/revisions/editionRevision.ts:55:17 - error TS2351: This expression is not constructable.
  Type 'Function' has no construct signatures.

55      return new EditionRevision()
                   ~~~~~~~~~~~~~~~

src/models/revisions/publisherRevision.ts:53:17 - error TS2351: This expression is not constructable.
  Type 'Function' has no construct signatures.

53      return new PublisherRevision()
                   ~~~~~~~~~~~~~~~~~

src/models/revisions/seriesRevision.ts:57:17 - error TS2351: This expression is not constructable.
  Type 'Function' has no construct signatures.

57      return new SeriesRevision()
                   ~~~~~~~~~~~~~~

src/models/revisions/workRevision.ts:53:17 - error TS2351: This expression is not constructable.
  Type 'Function' has no construct signatures.

53      return new WorkRevision()
                   ~~~~~~~~~~~~


Found 13 errors in 9 files.

Errors  Files
     2  src/func/entity-sets.ts:59
     4  src/func/entity.ts:43
     1  src/models/data/publisherData.ts:40
     1  src/models/revisions/authorRevision.ts:53
     1  src/models/revisions/editionGroupRevision.ts:53
     1  src/models/revisions/editionRevision.ts:55
     1  src/models/revisions/publisherRevision.ts:53
     1  src/models/revisions/seriesRevision.ts:57
     1  src/models/revisions/workRevision.ts:53
error Command failed with exit code 2.

[MonkeyDo]

A valiant effort !
This is going to make the ORM a lot easier to work with, indeed.

I do have a couple of suggestions and queries below:

[MonkeyDo]

Investigate whether these are also useful elsewhere, e.g. for form validation

I would say most likely. For example for import userscripts one day:
https://github.com/tr1ten/bookbrainz-userscripts/blob/amazon-import/src/amazon-import.user.js

[MonkeyDo]

Regarding the TS errors that are left:

• This expression is not constructable. Type 'Function' has no construct signatures.
  *sigh * What a pain. The first commit of the TS types already define Model.extend as deprecated, and suggest using TS classes instead. I did a quick test and that might work, but also requires basically rewriting every model, as you mentioned in a previous conversation I believe.
  At this point, I really think we should be moving to a properly typed ORM instead, sunk cost fallacy be damned. Sooo… maybe we @ts-ignore that error for now and add a clear comment as to why we're doing that?

• src/func/entity.ts dates and Argument of type 'unknown' is not assignable to parameter of type 'string'
  I think these might be resolved once we figure out the Parsed$Entity types so that we can use entityData as ParsedAuthor for each entity type in getAdditionalEntityProps

• Property 'query' does not exist on type 'typeof Model'.
  Regarding this line:
  

  bookbrainz-data-js/src/models/data/publisherData.js

  Line 39 in c29fa5a

  return Edition.query((qb) => {

  
  I think it's a mistake, and should be return new Edition().query(… instead

[kellnerd]

Sooo… maybe we @ts-ignore that error for now and add a clear comment as to why we're doing that?

I've added @ts-expect-error comments in 7843a11.

I think it's a mistake, and should be return new Edition().query(… instead

Good catch, changed in 10f9b55.

Now the 7 type errors related to the models are gone, only 4+2 remaining which should be solvable by two changes (which I still have to find).

[kellnerd]

Four out of the six type errors have been resolved by improving the types for parsed entities, which has resulted in one new type error (in the apparently unused create-entity.ts module):

src/func/create-entity.ts:108:51 - error TS2345: Argument of type 'ExtraEntityDataType' is not assignable to parameter of type 'ParsedEntity'.
  Type 'ExtraEntityDataType' is not assignable to type 'ParsedEdition'.
    Type 'ExtraEntityDataType' is missing the following properties from type 'ParsedBaseEntity': entityType, alias, metadata, source

108  const additionalProps = getAdditionalEntityProps(entityData, entityType);
                                                      ~~~~~~~~~~

src/func/entity-sets.ts:59:32 - error TS2339: Property 'model' does not exist on type 'EntitySetMetadataT'.

59   orm, transacting, derivedSet.model, [...unchangedItems, ...addedItems],
                                  ~~~~~

src/func/entity-sets.ts:83:41 - error TS2339: Property 'model' does not exist on type 'EntitySetMetadataT'.

83   const oldSetRecord = await derivedSet.model.forge({
                                           ~~~~~


Found 3 errors in 2 files.

Errors  Files
     1  src/func/create-entity.ts:108
     2  src/func/entity-sets.ts:59
error Command failed with exit code 2.

The new types also revealed an issue where the entityType property is used for two different things in different places: Once for the type of the entity itself and for the type of a series' items...
I still have to investigate how many places across the two repos this affects.

P.S. I fail to see how these type-only changes suddenly cause one test to fail...

[MonkeyDo]

Four out of the six type errors have been resolved by improving the types for parsed entities, which has resulted in one new type error (in the apparently unused create-entity.ts module):

The new types also revealed an issue where the entityType property is used for two different things in different places: Once for the type of the entity itself and for the type of a series' items... I still have to investigate how many places across the two repos this affects.

P.S. I fail to see how these type-only changes suddenly cause one test to fail...

Nice work !

I looked into the failing test and downloaded the test results file, which gives a bit more details of the expected vs. actual value.
The issue is unrelated to your changes, the timing is just a coincidence.
Turns out it's not expecting the privs key:

"failures": [
    {
      "title": "should return a JSON object with correct keys when saved",
      "fullTitle": "Editor model should return a JSON object with correct keys when saved",
      "file": "/home/runner/work/bookbrainz-data-js/bookbrainz-data-js/test/testEditor.js",
      "duration": 9,
      "currentRetry": 0,
      "err": {
        "message": "expected { id: 1, name: 'bob', …(17) } to have keys 'id', 'name', 'reputation', 'bio', 'createdAt', 'activeAt', 'typeId', 'gender', 'genderId', 'areaId', 'revisionsApplied', 'revisionsReverted', 'totalRevisions', 'type', 'revisions', 'titleUnlockId', 'metabrainzUserId', and 'cachedMetabrainzName'",
        "showDiff": true,
        "actual": "[\n  \"activeAt\"\n  \"areaId\"\n  \"bio\"\n  \"cachedMetabrainzName\"\n  \"createdAt\"\n  \"gender\"\n  \"genderId\"\n  \"id\"\n  \"metabrainzUserId\"\n  \"name\"\n  \"privs\"\n  \"reputation\"\n  \"revisions\"\n  \"revisionsApplied\"\n  \"revisionsReverted\"\n  \"titleUnlockId\"\n  \"totalRevisions\"\n  \"type\"\n  \"typeId\"\n]",
        "expected": "[\n  \"activeAt\"\n  \"areaId\"\n  \"bio\"\n  \"cachedMetabrainzName\"\n  \"createdAt\"\n  \"gender\"\n  \"genderId\"\n  \"id\"\n  \"metabrainzUserId\"\n  \"name\"\n  \"reputation\"\n  \"revisions\"\n  \"revisionsApplied\"\n  \"revisionsReverted\"\n  \"titleUnlockId\"\n  \"totalRevisions\"\n  \"type\"\n  \"typeId\"\n]",
        "stack": "AssertionError: expected { id: 1, name: 'bob', …(17) } to have keys 'id', 'name', 'reputation', 'bio', 'createdAt', 'activeAt', 'typeId', 'gender', 'genderId', 'areaId', 'revisionsApplied', 'revisionsReverted', 'totalRevisions', 'type', 'revisions', 'titleUnlockId', 'metabrainzUserId', and 'cachedMetabrainzName'\n    at Child.<anonymous> (node_modules/chai-as-promised/lib/chai-as-promised.js:302:22)\n    at Child.tryCatcher (node_modules/bluebird/js/release/util.js:16:23)\n    at Promise._settlePromiseFromHandler (node_modules/bluebird/js/release/promise.js:547:31)\n    at Promise._settlePromise (node_modules/bluebird/js/release/promise.js:604:18)\n    at Promise._settlePromise0 (node_modules/bluebird/js/release/promise.js:649:10)\n    at Promise._settlePromises (node_modules/bluebird/js/release/promise.js:729:18)\n    at _drainQueueStep (node_modules/bluebird/js/release/async.js:93:12)\n    at _drainQueue (node_modules/bluebird/js/release/async.js:86:9)\n    at Async._drainQueues (node_modules/bluebird/js/release/async.js:102:5)\n    at Immediate.Async.drainQueues [as _onImmediate] (node_modules/bluebird/js/release/async.js:15:14)\n    at processImmediate (node:internal/timers:466:21)"
      }
    }
  ],

The new column was merged into BB-site master branch only recently.
However, the CI workflow should in theory pull the bb-site repo and run a script to create the test database each time, so I suspect some cache issues.

/me opens a PR to fix the test in question

[kellnerd]

Ok, I think this is now ready for the final review. Lots of new type definitions, all relevant linter and type errors fixed, and the BBIQ consumer is finally working with these changes.

The remaining three type errors all originate from unused code, i.e. the createEntity function and other code which is only called from there as far as I can see:

bookbrainz-data-js/src/func/create-entity.ts

Line 62 in dfc71e4

export async function createEntity({

[MonkeyDo]

Great work, the ORM looks a lot cleaner now, and is much more usable when it comes to the importer project ! 🚀
It also paves the way for a future ORM change; having types will undoubtedly prove useful when converting.
Thanks for the hard work