#bookbrainz

Looking pretty good ShivamAwasthi !

I don't have much to add ot the conversation that took place, other than I agree the cards were a bit too big/too much for listing them.

The editor page looks great.

A small detail that could be improved is the way selected attributes are listed. I think maybe it's another job for a badge with a little "x" on the side (to remove the attribute, would replace the big "Remove" button). Alternatively, the multi-select component we use in the codebase is perfect for that: https://react-select.com/home

Lol, "Type 'Transaction<any, any>' is missing the following properties from type 'Transaction<any, any>': fromRaw, jsonExtract, jsonSet, jsonInsert, and 39 more."

Apparently @types/bookshelf depends on outdated knex typedefs...

Oh boy, more fun.

So either I'm going to fork the type defs too or I'm trying to survive without them.

i.e. it's a choice between getting sidetracked again or blindly trusting the JS parts in bb-data to work as expected.

kellnerd: There might be a way to override the typedef package, perhaps?

Yeah, I already had to adapt our tsconfig.json to use the regular @types package for our scoped package:

Right, that's the sort of thing I was thinking about

Now I'm trying to use a modified local .d.ts file instead

Can you not install the correct types package, and use a similar path to force resolving to that specific version?

(I have basically no experience with deno, not sure what's possible or not when it comes to package resolution)

i.e. force it to use a recent knex version for bookshelf types?

No idea

... how that works

This problem is unrelated to Deno

(since I have to build bb-data anyway)

At least I currently believe (and hope) that it is :)

Yeah, I get that it's an issue with old types packages, not deno

Let me check something

Ok, I'm going to prepare lunch now and will be back soon

This is what I was thinking of (obviously haven't tried it in the last minute, but…): https://deno.land/manual@v1.34.3/basics/import_...

Which would translate to something along those lines in deno.json:

{

"tasks": {

"bbiq": "deno run --allow-read --allow-env --allow-net src/bbiq.ts",

"consumer-test": "deno task bbiq consume -t",

"ol": "deno run --allow-read --allow-env --allow-net src/openLibrary/import.ts",

"ol-test": "deno task ol -t"

},

"scopes": {

"@types/bookshelf" : {

"knex": "npm:knex@^2.4.2"

}

}

}

Eek, that should have been a code block link, sorry

Yeah, this should do the trick for a native Deno project. Unfortunately this does not work with the npm compatibility layer, that's also why I had to resort to copying files from my local bb-data folder last week.

And I guess I've also been unclear about today's problem:

I'm currently working inside the bb-data repository where I wanted to use Bookshelf types to get a better understanding of what is happening there.

Ahhh, I see. Issues are in the bb-data repo, I misunderstood.

So rather than "unrelated to Deno" I should have said that I'm not using Deno at all for bb-data

But maybe the "overrides" property in package.json will help, I was only looking for solutions in tsconfig.json so far.

kellnerd: I'm still unsure where you are using the `@types/knex` package.

But if i remember correctly, the more recent version of knex that we use in our fork of booksheld comes with types already

Unfortunately not

Huh.

But yes, otherwise `overrides` is the noce package.json way to force resolution to specific packages

I will try that now, if the new types work we can still decide whether we want to upgrade our fork to already include types.

Yes.

Odd. Version 2.4.2 of knex does have a types directory: https://github.com/knex/knex/tree/2.4.2

I wonder if I missed something when I deployed our forked package of bookshelf

Wait, I misread: You're writing about @types/knex and I was writing/thinking about @types/bookshelf

It's a bit confusing: The initial error is about outdated Knex types which are shipped with @types/bookshelf

And of course they conflict with the types we have already implicitly installed via knex@2.4.2

Right. Then indeed I think you'll need to install the `@types/bookshelf` package and then overrride the version of knex in package.json (in bookbrainz-data)

rrrr

And indeed, afterwards reevaluate if we want to package those types in our bookshelf fork instead

> And of course they conflict with the types we have already implicitly installed via knex@2.4.2

Ah, I missed that sentence

> Overrides is used to support selective version overrides using npm, which lets you define custom package versions or ranges inside your dependencies. For yarn, use resolutions instead.

Since we are using yarn, it seems I have to use "resolutions" ("overrides" had no effect)

Yes, I forgot about that detail :p

Look at us, using three different JS runtimes !

Honestly I don't understand the syntax for yarn's "resolutions"...

If I understood correctly, that would mean I have to use "@types/bookshelf/knex": "^2.4.2"

which seems ambiguous...

I think you might want `"**/knex-d1": "^2.4.2"` instead

(reading https://github.com/yarnpkg/rfcs/blob/master/imp...)

:)

`"**/knex": "^2.4.2"`

Nothing happens, I guess I have to uninstall the package first.

Ah, might have to run yarn install again

Hmm, I've even manually deleted @types/bookshelf folder and it is still not doing anything...

(I'm not too familiar with yarn)

I'll eat something before I'll try again.

Same here

So I added the `resolutions` from above (`**/knex`) and then ran `yarn add @types/bookshelf -D` to add the package. looks like everything is resolved correctly; I see only one version of knex (the expected 2.4.2) and the sub-dependency in `node_modules/@types/bookshelf/node_modules/knex` is a symlink to the 2.4.2 version https://usercontent.irccloud-cdn.com/file/lUwGJ...

Building the project did work, but I didn't try running it past that. Gonna run the tests now and see if they pass

Well, all tests are passing. Didn't expect any issues considering we're replacing a types package dependency but it's always good to confirm.

What was the code you had that typescript was complaining about?

Never mind, running the typescript compiler does reveal 10 errors

I guess the same issues you had with `Transaction`, plus the other issues you might have run into eventually with `QueryBuilder`

I can get typescript compiler to pass, but it requires a couple of small changes in `node_modules/@types/bookshelf/index.d.ts`, which is basically the same as what you were suggesting before ("Now I'm trying to use a modified local .d.ts file instead")

For reference: https://www.diffchecker.com/KSmIyGKH/

This might be easier than forking and deploying our own version of `@types/bookshelf`: https://yarnpkg.com/cli/patch

Thanks monkey for testing this approach, so I would have to run an explicit yarn add instead of yarn install to apply the changes in package.json, interesting.

But since that isn't sufficient to solve the type issues, I'll probably follow the approach of using our own bookshelf.d.ts instead, for now stored inside bb-data (later we can still move it).

> For reference: https://www.diffchecker.com/KSmIyGKH/

Looks similar to the changes I've already tried locally :)

Right, Yep, sounds reasonable considering the mess this all is.

Wtf is this import X = require('x') syntax in this file by the way, looks like a mush of CJS and ESM...

I have converted that to `import {Knex} from 'knex';` instead.

Yep. i think that's also the cause of some of the incompatibility issues

incompatibility issues between knex 2.4 types and the types in @types/bookshelf, I mean

(That's what I also had to do for the knex update PR in bb-data, by the way.)

Right, I remember the CJS changes

Ok, it also works for my setup now. The only two type errors are from a local (in progress) conversion from JS to TS.

👍

monkey: https://github.com/bookbrainz/bookshelf/commits...

I'm successfully using these changes locally (yarn link)

So when you have time, you could publish the latest commits as a new version on npm.

kellnerd: Done, version 1.4.0 is out: https://www.npmjs.com/package/@metabrainz/books...

Thanks a lot :)

Is there anything else which needs to be done before https://github.com/metabrainz/bookbrainz-utils/... can be merged, monkey?

Not that it would be urgent

I was going to give it another thorough look tomorrow, but I don't anticipate there's anything left to do.

Great 👍

I had just thought about referencing one of the new files from bb-utils in a commit message in bb-data and was not sure how to permalink to it.

Ugh, trying to understand the usage of types across all BB repos is giving me a headache...

Apparently everybody has just defined types as needed "locally" and there are lots of duplication/inconsistency (e.g. entity types with and without 'id', complete and incomplete, sometimes lazy-loaded data is included, sometimes 'language' refers to an object, sometimes to the ID...)

I've just spend 15 minutes to understand whether an entity type which I am using needs an 'id' property (existing definition in bb-data) or not (my copied definition from bb-utils)

Since bb-utils definitely passes no 'id' and I'm still unsure whether 'id' is needed by other callers of the function in bb-data, I'm currently inclined to create a (third) new type definition where 'id' is optional...

Feels like https://xkcd.com/927/

Concrete example: https://www.irccloud.com/pastebin/mQMWnr8O/

Yeah, it's a real mess

monkey, it'll be nice to have some icons for relationship-types and identifier-types

probably a new one for the Admin Logs too. Haven't been able to find much on the react-fontawesome library

Lol, bookshelf's default inheritance approach is so awkward that the typedef creators decided to mark it as deprecated without telling the project maintainers: https://github.com/bookshelf/bookshelf/issues/1690