Refactoring Discussion [Input Appreciated]

[ChadBailey]

Video Speed Controller Refactoring

Call to action

Please read as much or as little of this as you like then reply with your honest feedback and recommendations.

You may view the refactoring progress here: https://github.com/ChadBailey/videospeed-refactoring - please feel free to open your ideas up as new issues on this project.

Intro

This is being posted as an issue for visibility purposes.

This is an invitation to be involved in the dialog around refactoring. I want to make decisions on if we should do it at all, to what depth, how to execute on it logistically, what tooling to target, and what benefits/value we expect or are targeting with the effort, etc.

This is the initial plan, which will evolve based on your feedback. Once we feel the plan is good enough to be 1.0 I'll enter the next phase of execution based on that plan. Any changes from that point will need to be pushed into the next phase.

Rationale

Optional reading, feel free to skip to the Refactoring Execution Plan section

Sometimes when refactoring is being proposed, people will jump to the defense of the software as if the person doing the proposal is saying the software sucks. They have the right to be concerned as they may have spent a lot of effort to learn the code base as is, and in some cases grown fond of it along with its flaws.

For that reason, I think it's important to point out this is all a positive thing. VSC has grown leaps and bounds over the years, and so too has its need for a quality code base. This quality code base I'm referring to is what empowers users to get in, make a change, and send it through to the end users with minimal fuss and protection against regressions. The easier VSC is to contribute to, the more growth we can expect as a result.

Background/Extra Context

Probably like most of you, I decided to get involved with VSC because I greatly appreciate the value it brings to my life. Originally, my plan was to offer up my professional experience to VSC as I perform an unusual role - I am a Software Quality Lead (depending on who you ask, maybe I'm a Test Architect, or I'm "the test guy"). Unfortunately, that never happened because I had to take an unexpected extended break from VSC for personal reasons.

My time away has finally come to a close and I'm able to refocus on this effort. It probably works out for the better in the end because I am better equipped to take it on now.

Refactoring Execution Plan

1.0 Goals:

1. Lift and shift codebase to a JS framework which supports importing and unit testing with as few changes to the codebase as possible (with necessary tooling i.e. Preact/Webpack/Babel)
2. Incorporate automated testing tooling - Jest
3. Create starter-pack of tests
4. Update documentation with how to kick off automated tests locally

1.1 Goals:

1. Add testing to pipeline (Github Actions) for automated PR rejection upon failing tests
2. Add enough additional tests to feel "comfortable-ish" with allowing the tests to act as the only reviewer of pull requests

   Important: This is merely a tool to form a mental model. Automated pull request merges are neither planned nor proposed.

1.2 Goals:

1. Add linting/style tools such as ESLint

Long-Term Aspirations:

1. Make contributing and reviewing contributions easier
2. Enable ability to deliver faster with automated regression tests
3. Re-architect options system to allow for moving currently hard-coded special handling for individual URLs into settings
4. Create rudimentary feature flagging system for launching experiments and gathering feedback
5. Add support/automated build and deployments for additional browser targets
6. Improve code base by implementing improved linting and style guides
7. Improve legibility of codebase by grouping common functions into classes
8. Improve legibility of codebase by shifting naming of classes/methods to match domain-specific-language
9. Create some level of documentation, preferably in documentation-as-code format
10. Docs targeting: end-users, devs, extension devs who wish to offer compatibility with VSC
11. Create integration tests which will continually validate VSC's functionality against top used websites
12. Create integration tests which validate compatibility with commonly paired extensions
13. TypeScript?

Long-Term Tech Stack Goals:

• Preact
• Webpack
• Babel
• Jest
• ESlint
• Docusaurus

[ChadBailey]

@igrigorik hope you don't mind me taking the initiative on this. If you have any concerns let me know and I can take it down until we work through them. I figured with how busy everyone is better to ask for forgiveness than permission.

[ChadBailey]

Since this has a long time horizon, I'm hosting this refactoring effort here: https://github.com/ChadBailey/videospeed-refactoring

At the time of writing this, it's a 1:1 mirror of the master branch here. Feel free to get involved with reporting bugs/submitting PRs etc.

Hopefully this can go relatively fast, I created the new repo so that I can package changes up into single larger PR's here while maintaining the ability to blow away my changes and reset if needed since I am still learning the JS build tools and anticipate making some mistakes. Any changes that happen meanwhile I will manually pull into the refactoring repo. I'll archive it when we've reached a point of stability on the build toolchain

[ChadBailey]

The initial version is working, if anyone wants to test and validate on their systems I would appreciate it!

(requires nodejs if you don't already have it, install the latest LTS version)

Just clone, npm install, npm run init:profile, then npm run chrome

Edit: skipped a step, oops 😅

[ChadBailey]

0.8.1 has been released, all initial goals have been accomplished and then some! Check it out #964

Keeping this open in the event anyone would like to comment, but feel free to respond to the new issue. I will close this once the refactoring effort is fully concluded.

[magnus-ISU]

Just want to comment that I also heavily refactored the code base for my fork and you might want to look at getting some of the improvements I added here too.

I will probably check some of the changes you make and see if I care about them. However, most of the stuff you're describing doesn't sound like it aligns with my goals (more/better functionality, less code) although some of it could be useful, for the most part it probably means the codebases will split completely.

The specific functionality I added was:

• Shift+scroll over a video changes that video's speed
• Changing settings immediately updates them so they can be used in other tabs for example without refreshing the tab
• Dark theme for extensions
• The size of the overlay customizable
• Distinguish uppercase and lowercase letters and symbol keys

Nice things I did to achieve this:

• Make it so options are only defined once (twice, but in the exact same way in a foreground process and the options screen I think). Adding an option in a single place keeps it consistent - in the code base as I remembered it, you needed to make changes in like 5 different places in esoteric ways which I hated and was the main motivation for me to rewrite most of the extension.

You already have all the code for how I achieved this, so I don't know what more I can say. I will go check out #964 now

[magnus-ISU]

If you want to make the extension work with other browsers, just check out the diff from here to the firefox port by codebycicle. https://github.com/codebicycle/videospeed

As I recall, the differences were very minor and the firefox version compiles to chromium. My version, for example, also compiles to chromium.

[ChadBailey]

Before you jump to conclusions about what I'm doing, I'd encourage you to take a look at the commit history here https://github.com/chadbailey/videospeed-refactoring and see what I've actually done so far. I think you'll see that our goals are aligned in that I'm trying to improve the architecture and make it easier to work on. Many times, that also means less code - just another way to frame solving the same problem.

[ChadBailey]

@igrigorik Can I have you take a cursory look at this and see if you agree with the direction?

If yes, I will get a pull request prepared and open it so we can go over the details. I don't want to invest the effort into getting it into a PR until the direction is agreed upon.

[igrigorik]

@ChadBailey, apologies. My GitHub notifications are a broken mess. If you're not hearing back, nudge me over email.

Doing a quick scan over the repo, I like what you've done and would love to see it merged 👍 I'll reserve the specific questions for later (e.g. I see we're adding a bunch of imports, but are those actually necessary/used—Tailwind?), once we have a PR to review.

Could you stage a PR for us to iterate on? I understand that it'll be a monster, but there isn't a simple way to piecemeal this.

[ChadBailey]

Thanks for the input on the direction @igrigorik !

No worries on the notifications - I have fallen into the same pit myself lol. Feel free to ping me via email as well if needed.

Regarding the imports, it's been a bit since I've last looked closely but best of memory some of that was experimental while I tested various things out and probably got left in.

I'll do my best to get all of that cleaned up before the PR. That's part of why I wanted some input before opening the PR because while it's not a ton of work to get it "pull ready", it's definitely not no work.

Ironically I just walked in the door of my temporary housing arrangement in Seattle so I'll get that done and open the PR once I get more settled in. I'm hoping to repair the reputation I've earned of leaving you hanging so I will try to be quick about it this time around lol. I'll use email if you don't see the PR.

You can track my progress by looking at the issues and git repo of the refactoring project (it will be archived once merged)

[ChadBailey]

Quick update... I plan to take make some progress on this by this weekend (2/26).

[igrigorik]

@ChadBailey could we get this over the finish line? Been using your branch for a while, looks stable — let's get it merged?