#bookbrainz

I'm finally back to work (TS migration) and already have a question:

Do you remember why the `|| orm` fallback is there monkey?

According to TypeScript and the original definition of the `orm` export this makes no sense as `knex` isn't a direct child of `orm`

Maybe there was a problem in an older version?

I can't recall, but I would guess it might be because of what the value of `bookshelf` is in this context: https://github.com/metabrainz/bookbrainz-data-j...

Let me see where else it could be used

Ah, or possibly the tests? https://github.com/metabrainz/bookbrainz-data-j...

Ok, so the problem is that the parameter isn't always the orm but can also be a bookshelf instance

(in models/area.js)

I guess I will rename the parameter to ormOrBookshelf in that case

I can't definitely find what the type of that first parameter ormOrBookshelf would be when called from the model instance versus when called from the test file

But if you remove that `|| orm` and try to run that test file you might find out :)

I'll give that a try :)

After changing the test's import to point to '../../lib/func/area' it fails (as expected)

The simplest fix would be to adapt the test and change the expected parameter of the source to be a bookshelf instance.

On the other hand, all similar functions expect an ORM instance and not a Bookshelf...

Unfortunate that the Area model has to pass a Bookshelf because it doesn't know about the ORM.

TS isn't well equipped to handle this kind of inline hacks to support both :/

Right, I guess that's the issue then, when used in one of the model's functions we only have access to the bookshelf object, not the whole ORM

Although…

Could you please try to call recursivelyGetAreaParentsWithNames with `bookshelf.bookshelf` here?

Identical result, only the test via parents() fails: https://www.irccloud.com/pastebin/jjkh3dMb/

The above happens 1) if I only remove `|| orm` or 2) if I remove `|| orm` and do the call with `bookshelf.bookshelf`or 3) if I only do the call with `bookshelf.bookshelf`.

Harumpf

So we either have to keep the parameter as type ORM | Bookshelf or change all calls to pass bookshalf or knex directly.

*bookshelf

Currently I have `const queryResult = await ((orm as ORM).bookshelf || (orm as Bookshelf)).knex.raw(rawSql);` as a somewhat ugly solution to satisfy TS :P

A typed version of the current mess is still an improvement

I thought I could do `const knex = orm.bookshelf ? orm.bookshelf.knex : orm.knex;` to let TS infer the correct type of orm, but that doesn't work...

TS already complains about the condition

Ha, I got it: `const knex = ('bookshelf' in orm) ? orm.bookshelf.knex : orm.knex;` works

Ah great

I forgot about the correct syntax for this :)

> @param {string} areaId - The entity model name.

Hehe, I guess the description is wrong and this is is the MBID?

MBID = BBID in this case of course

This was finally a piece of code which I could fully type (as no Bookshelf model was involved directly): https://github.com/kellnerd/bookbrainz-data-js/...

Much better !

Still a pain to have to do that, but at least it's clearer now

Everything in bb-data besides the models has now been migrated to TS 🎉

Well done ! 💯

I will also convert the models later, but there aren't many types to add unfortunately.

Probably a lot of repetitive work, as far as I can see there will only one change per model: https://github.com/metabrainz/bookbrainz-data-j...

It's done™ : https://github.com/kellnerd/bookbrainz-data-js/...

There are still a few type errors which can probably be fixed by checking the Bookshelf documentation for alternative usage patterns.

i.e. by replacing `Model.forge(...)` with `new Model(...)` which is equivalent as far as I understood.