chore: more cleanup

[aweebs]

Some more cleanup commits stacked on each other.

Goals that this PR builds towards:

1. No more @ts-expect-error exceptions - these can be avoided by typecasting where necessary. It's less safe to completely ignore any static compiling error on a line than to resolve an error with a typecast. Linting rule coming in future PR.
2. No more unnecessary as casts - linting rule incoming in later PR once these are all removed.
3. typecheck script runs successfully

Recommend reviewing the changes commit by commit

[ferferga]

I'm not a fan at all of the changes to the data interface - to me, it seems like another way to remove as just for the sake of it. In your other PR I didn't mind it in the metadata pages because the fixes in the page were good and it was a nitpick really. However, modifying files purposefully just for this is, in my opinion, "polluting" the commit history (I'm more concerned about the quality and usefulness of our commits since our repo starting getting bigger and bigger and it's no longer a 1 second to clone 😣) without any real improvement, when the real improvement would've been to directly migrate all those files to Composition API.

I'd love to keep the rest of the runtime checks/handles you added, but please also take the extra step (which I know is cumbersome and annoying at times) to migrate them to Composition API 🙏.

[ferferga]

@aweebs By the way, as mentioned here, you can chat with us in Matrix if you want to get quick replies to your questions and in a more informal way

We're bad at doing detailed roadmaps or in publicily document our preferences and conventions (for commits, for instance) because we get all of them resolved in chat (between @ThibaultNocchi and me) and right now we want to get stuff done instead of focusing in proper documentation (which is also much less appealing to work in :P). In the meantime, hopefully we can have you around in Matrix so you don't end up doing extra work that could have been saved with a question in chat beforehand :P

[aweebs]

I'm not a fan at all of the changes to the data interface - to me, it seems like another way to remove as just for the sake of it. In your other PR I didn't mind it in the metadata pages because the fixes in the page were good and it was a nitpick really. However, modifying files purposefully just for this is, in my opinion, "polluting" the commit history (I'm more concerned about the quality and usefulness of our commits since our repo starting getting bigger and bigger and it's no longer a 1 second to clone 😣) without any real improvement, when the real improvement would've been to directly migrate all those files to Composition API.
I'd love to keep the rest of the runtime checks/handles you added, but please also take the extra step (which I know is cumbersome and annoying at times) to migrate them to Composition API 🙏.

I'm conflicted about this. I think these Data interface changes do represent some amount of progress since they help with type safety, but agree that they're not significant at the moment. There's 15 components that were updated, and making the full transition to the Composition API will take me a while, but I'm happy to continue forward on that path as separate tasks/PRs.
I think this is a good example of incremental progress vs perfection.
In terms of git history pollution, I don't think worrying about repo clone times is a valid concern. The git history will only increase, and this repo is still very small comparatively. I also don't believe that this obstructs the history - it's a single commit and easy to go backwards a step when looking at the history for operations like a git blame.

@aweebs By the way, as mentioned here, you can chat with us in Matrix if you want to get quick replies to your questions and in a more informal way

I'll definitely join the Matrix chat, thanks for pointing that out!

[ferferga]

@aweebs

I think these Data interface changes do represent some amount of progress since they help with type safety

They don't. The rest of the changes in some of those files (because even not all of the files are modified beyond the Data interface, like QueueButton.vue) add type safety with if checks and the use of the nullish coalescing (??) operand. However, the data interface itself doesn't change anything runtime-wise. The only change is the aesthetics of how those types are defined for the compiler. And just change the aesthetic for the sake of change aesthetic without any real improvement (I don't like as either, but using it makes for much less verbose code than a new interface) while the real improvement in type safety (aside from the if checks and null coalescing) is to use Composition API, which has better type-checking support from Volar during development, cleaner definition of types with defineProps, etc etc... It's the real solution, this is, as with everything cosmetic, just a highly subjective patch. And the effort of doing it objectively better by migrating to Composition API is not much bigger (albeit I know it's sometimes frustrating).

And yes, I know the repo will grow, the point is not that, but the fact that the commits are as relevant as possible. And, as said before, the interface approach is sbjectively relevant, while Composition API is objectively relevant with not much more effort. When you do a git blame, you must go commit by commit. Going commit by commit takes time, and takes more time if you have even more commits. Style commits for instance are not useful at all when debugging, but commits where a file was migrated to Composition API is 100x times more useful.

I'm sorry, but this is a hard no for me. We've been carrying some technical debt issues specifically because of lazy commits and workarounds that "we can change/refactor in future PRs" (and I'm the first culprit of doing that) and we end up completely forgetting or abandoning. The metadata editing section and library view, as you saw, require heavy refactoring just for this sole reason, in my opinion. Bigger stuff of course will still be affected and will require revisits in the future and multiple PRs. But I'd like to get PRs merged in the best possible state within a reasonable scope (that's also why vite is taking so long to get merged, because I could've started working in master directly and leave the client broken for a lot of time).

It's true the LoC for this PR will get much bigger with the migration, but I don't mind reviewing them here if you don't want the extra hassle. However, if you prefer another PR, just cherrypick the data interface commit to another branch and start migrating those modified components to Composition API and amend the name afterwards. You can also do it in different commits when the scope of those components is different so, as said above, the git blame is more useful.

TL:DR: The Data interface is just a subjective middle-ground for improvements in DX when a migration to Composition API is the real deal and this just seems a lazy (with all due respects, because I know finding all the occurences of as takes some time) approach.

[aweebs]

@ferferga I've removed the Data interface change commit, as well as the commit for deleting PlaybackInfoCard. The rest of the feedback had been incorporated.

[ferferga]

As some files are conflicting with the implement video playback PR (#1878) and #1889, if you don't mind, I'm going to progressively cherry-pick commits that I'm sure that don't conflict with those, then merge those PRs, and then fix the conflicts of the remaining commits from this PR.

But here you have the greencheck of your changes :)

[aweebs]

This change is necessary since calls like getting devices or apikeys also end up coming through this function, see my comment in axios/index, but they won't have a valid Id, and this was previously breaking those calls. Now if there no id the non-reactive value is still returned without being cached so those calls will continue working.

[ferferga]

@aweebs I like your fallback when the item doesn't have an id.

About the axios interceptor, ideally we should strictly check for everything before calling this method in the interceptor. Previously in the item store we had all the API methods (some, not all of them), which was a maintenance burden. The interceptor-way of working was a middle-ground I came up with to ease the maintenance burden, however it didn't end up working because the Vue 2 limitations.

Just saying all of this if you want to take another challenge :): Investigate all the endpoints that returns BaseItemDto or BaseItemPerson and filter them properly.

[aweebs]

I'll add it to my list of things to work on :)
I also added a fairly descriptive TODO to the interceptor so that anyone else who comes across it is also aware of the limitations and that there is work to be done there.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[jellyfin-bot]

Cloudflare Pages deployment

Latest commit	0048cc1
Status	✅ Deployed!
Preview URL	https://707983d6.jf-vue.pages.dev
Type	🔀 Preview

View build logs
View bot logs