#bookbrainz

I have wrapped things in the Card component in the pages I have created, for example, the Admin Logs Page and the Relationship Types page. I'm aware that we only used Cards for the entity editor, and not any of the pages. If it is something which might lead to non-uniformity, then I can remove the card component and make it look more like the Revisions page or the Collections page. Personally, I like the header the Card

component provides for the title of the page, that is why I get naturally inclined to use it

Although I dont think we really have any strict uniformity in place, with respect to how it should look depending on whether its a form or a display page. Because for example, the Collection creation form does not use the Card component.

monkey: Wow, it looks like yesterdays type issue with the aliases is indeed unsolvable because it hints that there is a real *bug* in the existing code 🤯

Here we are passing an array of aliases without 'id' props but the function expects aliases with IDs.

If I understood correctly, this means that this function should only be called with aliases which were already inserted into the DB and hence have an ID.

But inside the createImport function we are only trying to create an AliasSet, not the aliases themselves!

I'm a bit shocked to be honest, because I had assumed that this code had been tested in 2018, I think I've even seen screenshots of the importer UI with pending imports!?

Does that mean I cannot trust any of the existing code?

I'm also wondering why my previous tests with BBIQ always failed only at the last SQL statement https://github.com/kellnerd/bookbrainz-data-js/...

I don't know if any of this code was tested. It's from just before my time working on BookBrainz

Interesting

(Not that the time of failure made a difference, since all actions are inside a transaction which got rolled back every time)

Ok, so we basically have to assume that the whole importer is untested from now on...

> If I understood correctly, this means that this function should only be called with aliases which were already inserted into the DB and hence have an ID.

Just read the `updateAliasSet` function, it looks to me like it also expects aliases without ids.

It inserts new aliases in the database as well as creating a new set, if I read correctly

Where does it insert them?

createNewSetWithItems?

Yes

Aha, very opaque...

But as you say, with the imprecise types with or without IDs, it forces us to make sure the new laiases have no ids: _.omit(ident, idAttribute)

Which…🤮

Ok, so the only case where we actually check for 'id' is https://github.com/kellnerd/bookbrainz-data-js/...

In that case I could make the 'id' optional at least

(and do the necessary code adaptations elsewhere)

*resulting necessary

Sorry, the prod servers are on fire, I'll be back in a bit

No problem, you've already answered my most important question

(the one where I had assumed the contrary about alias creation happening or not)

OK, fires are out, I'm back :)

> so the only case where we actually check for 'id' is…

Yes, that looks correct

Looking at those types… woof.

Why not `type FormAliasWithDefaultT = FormAliasT & { default: boolean};`?

And contrarily, we might need a new type `NewAliasT` that does not have an id

In `updateAliasSet` we would have `oldSet?: FormAliasWithDefaultT[]` and `newSetItemsWithDefault: NewAliasT[]`

Yeah, we should agree on a certain terminology and which type definition for an entity should be the base.

Currently we also have:

... where I'm currently unsure whether Insertable should remove 'id' or make it optional.

First question is whether Form* and Parsed* have any meaning besides their original use cases.

ParsedAlias = FormAliasWithDefaultT - Id basically, which lets me think we don't need either of Form* and Parsed* in the final type names.

Once we are using types in more places, they will probably useful in other areas of the codebase as well.

So my suggestion would be to start with a type named Alias or AliasT (useful if there is a class and/or React component of the same name) and everything which needs more or less (special) properties gets a longer type name.

I think it probably makes sense to strip the LazyLoaded part from the basic type, but I'm unsure whether 'id' should be part of the basic type or not.

Agree with a simple `AliasT` with basic types (i.e. without id) and extend from there. That's the sort of composability that will make it more useful in the other repos

> but I'm unsure whether 'id' should be part of the basic type or not.

I reckon it does make sense to remove it. We rarely use the id, as you pointed out in the alias set update code it's only used once for example

I think this is where ParsedAliasT comes from: Alias as parsed from the DB by the ORM, meaning for example it will have an id

The name is cryptic though.

ParsedAlias is my creation, it's a WIP type for the importer project :)

It's something which gets inserted into the import queue

But I agree, the name is not good since I didn't knew exactly how it relates to the ORM when I defined it.

Ahhh

OK, makes more sense now thanks

Not I think the name is OK in that context

Having AliasT defined without 'id' also has the nice side effect that "lazily" typed functions wouldn't require an 'id' attribute which they may not need (as it happened here).

So what would be an alias that was previously saved and retrieved from the DB, with the extra id attribute? Simply `AliasWithIdT` N

-N + ?

^ i.e. one would have to explicitly choose a type which has an 'id'

That or FetchedAliasT?

I think that's reasonable. Expect a basic Alias, but if your use case requires an alias with ID choose the right specific type.

🤔

Yeah, in case there's more than just an extra id field

Let me have a quick look what other libraries do...

The name is good, but also requires some context to understand. I don't have a better suggestion

uses Selectable

i.e. after the SQL commands

Oh, I see

I would understand the wrong thing, given `SelectableAlias` and no context :)

Ah, that's convenient, the Generated type

Yeah, Selectable isn't as simple to understand as Insertable :)

Yep

My only fear about Generated is that it's definition may be a bit complex...

By the way, Kysely is the new project by the former Objection.js lead dev, it may be interesting for us once we again look into replacing Bookshelf.js

Objection development has also continued during the last months, for the record

That's good to know

Because it's starting to feel pretty capital to move away from Bookshelf…

pressing even

Yeah, I had again realized why having the Bookshelf types installed does not help us much with typing our ORM.

It's the unorthodox inheritance concept using .extend()

yeah, such a mess…

Lol, after searching for the Generated type in the kysely GH repo for five minutes without success, I've finally decided to clone and search locally 🙄

Yeah, I gave up too :D

Ok, that's above my head

It's only a marker type which has to be consumed elsewhere to have effect.

There's some pretty magical typescript-fu in that file 😵‍💫

Reminds me of https://github.com/microsoft/TypeScript/issues/...

🤣

There are lot of languages which are Turing-complete, but nevertheless shouldn't be used to solve arbitrary problems.

One of my favourites: https://en.wikipedia.org/wiki/Brainfuck

Back on topic: I guess that I should start with refactoring the existing conflicting types, the only problem is that they are used across three different repositories.

A common problem which I have.

Ahhhh, brainfuck <3

I attempted to lear it a long time ago

I'd say refactor the types you need for now (which will be a lot of them!), we'll lukely need to completely reevaluate our approach when we change ORMs

I agree

Already having a decent type coverage also makes me feel more comfortable about doing a huge refactoring such as replacing the ORM.