A few codebase thoughts and feedback

[toasted-nutbread]

As mentioned in #331 (comment), here is some of the other feedback I can give based on my recent work:

1. Avoid using object for Map<>s (assuming cross-frame serialization is not required).
   I didn't notice a lot of this, but here was one example.
2. A few test scripts have had the --write command line option removed.
   This option existed because these compare massive JSON files which are tedious or infeasible to update by hand when a valid code change occurs which may modify them. I would recommend re-adding this at some point if/when such a scenario is encountered; I did have to do this during the process of adding type annotations.
   • anki-note-builder.test.js
   • translator.test.js
3. I would recommend removing Dexie.
   Yomichan used Dexie a very long time ago, and I removed it because it was a huge performance bottleneck; using IndexedDB directly was dramatically faster. Most of this stems from Dexie's APIs not being direct mappings of IndexedDB, which makes sense since it's trying to make it "easier", but in the process some performance sacrifices are made.
   But more than that, since the core application already uses IndexedDB directly, it feels weird to be using two database interfaces to interact with it. My general thoughts in the past have been that less third party libraries is better.
4. I would strongly recommend using squash commits.
   Perhaps me mentioning this is a bit too late since my PR has already been merged, but you'll notice that most Yomichan-era history used squash commits. I believe that a rebase+squash workflow simplifies a lot of things compared to a merge commit workflow. The individual commits of a branch/PR are generally useless noise when viewed in the context of the overall project history, and as such, they should be squashed into one commit.
   For example, the history of my type annotations branch probably has 100 or so commits. The individual commits are largely irrelevant to the final diff vs the master branch.
   Original discussion of this process can be found here: Merge strategy for pull requests FooSoft/yomichan#480

[djahandarie]

Thank you for the diverse comments 🙏

Avoid using object for Map<>s (assuming cross-frame serialization is not required).
I didn't notice a lot of this, but here was one example.

Sounds good. Is there a way to lint for this pattern?

A few test scripts have had the --write command line option removed.
This option existed because these compare massive JSON files which are tedious or infeasible to update by hand when a valid code change occurs which may modify them. I would recommend re-adding this at some point if/when such a scenario is encountered; I did have to do this during the process of adding type annotations.

Thanks, I wasn't aware of why it existed. I don't quite remember why I / someone removed it, maybe something about the implementation was hard to port.

I would recommend removing Dexie.

FWIW, it (or rather dexie-export-import in particular) is only used for the IndexedDB import/export functionality, not anything else, so I'd like to think it doesn't affect performance (but this may not be true given #329 -- that said I haven't debugged that yet to determine if it's really dexie's fault or something else).

BTW, I had originally written the import/export manually by hand in #115 with no dexie, but the contributor who came in and did a more complete/robust implementation decided that it'd be better to rely on a library which does this as opposed to do it by hand. I understand the argument and did not care enough to tell them to redo it or redo it myself. Also, this architecture does have the benefit that it gets saved to a file, which is quite portable, as opposed to my version which needed two copies of the extension running in the same browser which is a much more limited usecase.

Anyways, I'd better totally open to a non-dexie implementation of this, but basically it would need someone who really cares about it to go do it I think.

[toasted-nutbread]

Sounds good. Is there a way to lint for this pattern?

At this point, I would say the main deterrent is that type annotations are required. Map<string, T> is easier to type than {[key: string]: T} and more idiomatic for typed code. I'm not aware of any explicit linting rules, but you can search the regular expression \{\[\w+: string\]: in the codebase to find some instances. PRs are also an opportunity I suppose.

[djahandarie]

I would strongly recommend using squash commits.

Personally, since we have the higher level concept of PRs, and those are what are used to generate release notes, I feel like there is not that much downside to having more granular git history, as it preserves more information about what the developer was thinking, and how something might have gotten messed up (e.g., they were developing on an old version, then messed up the merge commit when updating their branch, etc); also it preserves the original commit signing. But anyways, that stance only makes sense in an environment where developers are at least kind of trying to maintain logical commit sizes and good commit messages in their original branch, which given the reality of an open-source project with tons of drive-by contributors, is not really true, as people have different styles, and some people may make tons of tiny commits with meaningless commit messages. I don't think it's worth the maintenance burden of forcing people to rework their git histories, especially as many people do not know how to do that well.

So, I switched the merge rule for the branch protection to be squash and merge. Thanks for the suggestion!

@toasted-nutbread By the way, would you be interested in being a code reviewer for the project? I think you can do a much better job than I can when it comes to preserving the actual quality of the codebase. We could have you as an optional reviewer (I guess I would tag you on things where I think it would be helpful), or a required reviewer for all PRs, whatever you prefer!

[toasted-nutbread]

I am open to it, although be warned that I cannot guarantee time commitment and that I frequently leave feedback that can sometimes be unexpectedly in-depth, although I suspect that's kinda what you're hoping for.

[toasted-nutbread]

Going to close this for now, as my feedback has largely been discussed/addressed. Feel free to tag me again though if something comes up.