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.
2023-07-31 21251, 2023
monkey
The editor page looks great.
2023-07-31 21251, 2023
monkey
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."
2023-07-31 21251, 2023
kellnerd
Apparently @types/bookshelf depends on outdated knex typedefs...
2023-07-31 21215, 2023
monkey
Oh boy, more fun.
2023-07-31 21239, 2023
kellnerd
So either I'm going to fork the type defs too or I'm trying to survive without them.
2023-07-31 21235, 2023
kellnerd
i.e. it's a choice between getting sidetracked again or blindly trusting the JS parts in bb-data to work as expected.
2023-07-31 21256, 2023
monkey
kellnerd: There might be a way to override the typedef package, perhaps?
2023-07-31 21213, 2023
kellnerd
Yeah, I already had to adapt our tsconfig.json to use the regular @types package for our scoped package:
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.
2023-07-31 21208, 2023
kellnerd
And I guess I've also been unclear about today's problem:
2023-07-31 21254, 2023
kellnerd
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.
2023-07-31 21209, 2023
monkey
Ahhh, I see. Issues are in the bb-data repo, I misunderstood.
2023-07-31 21211, 2023
kellnerd
So rather than "unrelated to Deno" I should have said that I'm not using Deno at all for bb-data
2023-07-31 21224, 2023
kellnerd
But maybe the "overrides" property in package.json will help, I was only looking for solutions in tsconfig.json so far.
2023-07-31 21233, 2023
monkey
kellnerd: I'm still unsure where you are using the `@types/knex` package.
2023-07-31 21207, 2023
monkey
But if i remember correctly, the more recent version of knex that we use in our fork of booksheld comes with types already
2023-07-31 21201, 2023
kellnerd
Unfortunately not
2023-07-31 21208, 2023
monkey
Huh.
2023-07-31 21224, 2023
monkey
But yes, otherwise `overrides` is the noce package.json way to force resolution to specific packages
2023-07-31 21217, 2023
kellnerd
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.
I wonder if I missed something when I deployed our forked package of bookshelf
2023-07-31 21213, 2023
kellnerd
Wait, I misread: You're writing about @types/knex and I was writing/thinking about @types/bookshelf
2023-07-31 21218, 2023
kellnerd
It's a bit confusing: The initial error is about outdated Knex types which are shipped with @types/bookshelf
2023-07-31 21248, 2023
kellnerd
And of course they conflict with the types we have already implicitly installed via knex@2.4.2
2023-07-31 21231, 2023
monkey
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)
2023-07-31 21252, 2023
kellnerd
rrrr
2023-07-31 21253, 2023
monkey
And indeed, afterwards reevaluate if we want to package those types in our bookshelf fork instead
2023-07-31 21233, 2023
monkey
> And of course they conflict with the types we have already implicitly installed via knex@2.4.2
2023-07-31 21233, 2023
monkey
Ah, I missed that sentence
2023-07-31 21228, 2023
kellnerd
> 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.
2023-07-31 21202, 2023
kellnerd
Since we are using yarn, it seems I have to use "resolutions" ("overrides" had no effect)
2023-07-31 21215, 2023
monkey
Yes, I forgot about that detail :p
2023-07-31 21230, 2023
monkey
Look at us, using three different JS runtimes !
2023-07-31 21205, 2023
kellnerd
Honestly I don't understand the syntax for yarn's "resolutions"...
Nothing happens, I guess I have to uninstall the package first.
2023-07-31 21243, 2023
monkey
Ah, might have to run yarn install again
2023-07-31 21236, 2023
kellnerd
Hmm, I've even manually deleted @types/bookshelf folder and it is still not doing anything...
2023-07-31 21242, 2023
kellnerd
(I'm not too familiar with yarn)
2023-07-31 21200, 2023
kellnerd
I'll eat something before I'll try again.
2023-07-31 21210, 2023
monkey
Same here
2023-07-31 21218, 2023
monkey
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/lUwGJC8…
I guess the same issues you had with `Transaction`, plus the other issues you might have run into eventually with `QueryBuilder`
2023-07-31 21202, 2023
monkey
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")
This might be easier than forking and deploying our own version of `@types/bookshelf`: https://yarnpkg.com/cli/patch
2023-07-31 21202, 2023
kellnerd
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.
2023-07-31 21202, 2023
kellnerd
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).
I was going to give it another thorough look tomorrow, but I don't anticipate there's anything left to do.
2023-07-31 21221, 2023
kellnerd
Great 👍
2023-07-31 21242, 2023
kellnerd
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.
2023-07-31 21223, 2023
kellnerd
Ugh, trying to understand the usage of types across all BB repos is giving me a headache...
2023-07-31 21223, 2023
kellnerd
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...)
2023-07-31 21212, 2023
kellnerd
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)
2023-07-31 21227, 2023
kellnerd
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...
monkey, it'll be nice to have some icons for relationship-types and identifier-types
2023-07-31 21200, 2023
ShivamAwasthi
probably a new one for the Admin Logs too. Haven't been able to find much on the react-fontawesome library
2023-07-31 21204, 2023
kellnerd
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
