chylex / Discord-History-Tracker

Desktop app & browser script that saves Discord chat history into a file, and an offline viewer that displays the file.
https://dht.chylex.com
MIT License
464 stars 81 forks source link

Turn browser version into a soft fork of the desktop app. Also automatically update website clientside with GH AJAX CORS. #236

Closed will-ca closed 10 months ago

will-ca commented 10 months ago

So… I'm sure this changeset probably looks scary. But it's broken up by commit, and most of that is just moving or copying files across the desktop and browser branches.

If you only count the two commits where I actually changed any code, it's only around 150 lines JS and 600 lines HTML/JSON, and most of that is just moving stuff from the website/wiki into the repository to make it easier to maintain.

…Takes a while to explain, but the stuff itself is simple, and easy to verify.


Tracker

The only part of DHT that seems to break regularly due to Discord changes is discord.js.

This PR introduces a new branch, simply called main-browser.

This branch is based on the full history from the master branch, but with some extra git-tracked file moves and commits to structure it like the old master-browser-only branch, using a ported version of the the browser-version GUI and state code.

So, discord.js, dom.js, settings.js, and bootstrap.js aren't just backported from the desktop version. They are the exact same code, and git knows about it too. And meanwhile gui.js, state.js, etc. are copied from the browser version but ported forwards to work with the desktop internal API.

  1. This brings the browser version up to date with the desktop version in compatibility updates.
  2. This makes it easy to automatically apply future fixes from the desktop version to the browser version by simply running git rebase or git cherry-pick.
  3. This hopefully eliminates or greatly reduces the need to test the browser version separately when maintenance fixes are done to discord.js, as the exact same code will already have been tested for the desktop version.

Make some change on the desktop version:

$ git switch master

$ echo -e '\nconsole.log("Haha, New Change to Desktop App!")' |
    tee -a ./app/Resources/Tracker/scripts/discord.js |
    tee -a ./app/Resources/Tracker/scripts/dom.js |
    tee -a ./app/Resources/Tracker/bootstrap.js
$ git commit -am "New change that used to be annoying to port to the browser version."

$ webbrowser # Do testing here on desktop version.

Automatically merge it into the browser version:

$ git switch main-browser

$ git cherry-pick master
$ # OR:
$ git rebase master

$ git tag "That was easy."

Website

Additionally, this PR also makes the website automatically fetch current data for the script contents, release notes, and old versions using AJAX CORS requests to the live GitHub repository.

(Krashboard does this too without the CORS. Well, it would, if I were to get around to releasing it.)

Simply update VERSIONS.json and run build.py (as normal) while pushing a new version. The web stuff should automatically update clientside, without having to ever rebuild/reupload or test the website on each version.

Preview link. Version History. (Remind me to take this down too so nobody gets confused.)

Depending on your preference, this AJAX CORS may be a web design sin. Well, if DHT's goal were to be a website, I wouldn't dare to suggest it. But DHT's goal is to save Discord messages, so IMO anything that lets the focus stay on that is fine provided it works in 99% of cases.

Note that the viewer file can no longer be used directly on the website, presumably because raw.githubusercontent serves it with Content-Type: text/plain. Downloading it as the instructions say still works.

I would have made this a separate PR, but as this is a fully divergent branch from master-browser-only I don't know where to target it. Feel free to pick out or exclude anything.


Testing

I tested all features of the manual browser version, and ran track.js through my code editor's static analysis.

I have not tested the JS minification, as I do not currently have uglifyjs available. I have tested the userscript. I tried to test the bookmarklet, but I think it requires minification because otherwise the first // comment just negates the entire rest of the script. But I don't see any reason why it won't work.

I have tested the website changes on Gecko, Blink, and WebKit, including with simulated slow network conditions and with Javascript off (where it just links to the repository).

There are three or four inline URLs on the website that cannot be set correctly unless the PR is merged, because they have to point to a live GitHub repository. Currently they point to my fork; They can be easily found and changed by searching for "TODO".

Lots of code lines are changed, but most of it is copied and pasted (and/or find-and-replaced, for indentation) from the browser version. When reviewing the code, I recommend going commit-by-commit.

The first couple commits only move existing files around without any functional changes, which you may easily verify, and only minor changes were necessary afterwards to update the browser-version code where the two branches had diverged.

Be mindful that this branch represents an entirely divergent commit history from master-browser-only, based instead on master. That is the point; To turn it into a soft fork of the desktop version. So… Instead of regular merging, it would have to be pulled and then pushed to a new branch, merged into a new branch based on master, or push --forced into master-browser-only.


Closes #230. Closes #234. Hopefully makes future issues easier to resolve as well.

will-ca commented 10 months ago

Reviewing tips:

  1. The first commit only deletes and moves files. Skim git show --name-status 787dce to quickly verify this.
  2. The second and third commits only copy files from the old master-browser-only branch over to here.
  3. To verify this, first make git list all the files that changed from the first to third commit, then diff the third commit against the master-browser-only branch to confirm that none of those files have changed: git diff master-browser-only faba9d -- $(git diff --name-only 787dce faba9d). (There should be no output. IDK How to do command substitution on Windows, so you an also just run the --name-only separately and copy and paste its output for the diff.)
  4. The fourth commit only changes indentation to tabs. Run git show --ignore-space-change f0e312 to verify this. (The diff should be blank.)
  5. The fifth commit again only moves files around. So skim git show --name-status 031e66 to verify this.

Everything afterwards should be simple enough to review. ba4dcfa8351efca86a655e9bfaa03b369aa89749 is the only one with tracker code changes, so read that instead of the net diff. 752d7edf26bf8e25a6bf688c483abb8ea106e3f9 likewise for the website.

TheTechRobo commented 10 months ago

FYI, I'm not sure if you meant to do this, but the pull request is currently targeted at the master branch (i.e. merging will change the master branch, not create a new one).

will-ca commented 10 months ago

@TheTechRobo Yep. It can't be merged, because it's a new commit history divergent from master-browser-only. It has to be made into a new branch, or push -f'd.

…I don't think there's a way for me to make the PR automatically create a new branch? Is there? I assumed Chylex would have to make a new branch separately (or git reset master-browser-only to master so this can be retargetted to master-browser-only and then merged, or whatever).

TheTechRobo commented 10 months ago

Yeah, the new branch will probably have to be added manually.

will-ca commented 10 months ago

Updated based on latest desktop master using cherry-pick method. This took a couple minutes longer than a typical compatibility update should, because it involved the DHT internal API changing, as a result of discovering #235 while I was preparing this.

The merge conflicts are fine. Changes to master only need to be cherry-pick-able into this branch, but this branch doesn't need to be mergeable back into master. It's just because files were changed in master which are deleted in this branch.

chylex commented 10 months ago

Thanks!

Regarding keeping history from the desktop branch, I don't think that's necessary for a few reasons:

  1. Applying patches from the desktop version is not much of an issue; one problem I found is my IDE gets confused about which file to patch when the tracker and viewer has a file with the same name, which can be resolved in easier ways.
  2. The release notes I apparently already gave up on some time ago and instead it just links to the commit history (https://github.com/chylex/Discord-History-Tracker/wiki/Release-Notes), so that's already zero-maintenance and rewriting the history would make it impossible to know what's in those versions.
  3. As far as I know, git only tracks renames by contents and not history, so with enough changes I'm not sure if cherry-pick would still work. It might make more sense to move app/* into the root and then move browser-only version's resources into the Resources/ folder if needed.

I'm not going to merge the website changes, if I were to automate it I would do it with CI or GH webhooks, but it's not really worth the time. Even if I'm in control of the repo, it doesn't sit right with me to make requests to a third party.

It also won't save much on manual testing, mainly because minification breaks things, so any change ported to the browser-only version needs to be tested with minification. This has diverged from the desktop app even further, because minification has been removed from the app entirely (and replaced with a one-line fetch-eval that can only be done because of the app's integrated server).

So I'm going to merge most changes into master-browser-only. I can try to preserve authorship, but if you want you can reopen the PR against master-browser-only in the state it is now, and GH lets you give me permission to edit the branch in your fork.

will-ca commented 10 months ago

…Okay, yeah, that's probably the most sensible approach. The rebase/cherry-pick workflow sounded a lot cleaner before I saw just how many files it involved restructuring, and I have it working well now but I do wonder if it would decay over time with more stacked cherry-picks or repeated rebase rewrites.

I did notice the status of the release notes, and copied the commit messages for the absent versions into VERSIONS.json.

Kudos for integrity on the website. I do think serverside generation or CI is probably the "right" way to do it.

If minification is a source of breakages, I might question its value? The userscript and the manual method don't need it, and the bytes it saves are probably going to be eclipsed by what Discord itself uses.

So I'm going to merge most changes into master-browser-only. I can try to preserve authorship, but if you want you can reopen the PR against master-browser-only in the state it is now, and GH lets you give me permission to edit the branch in your fork.

Trying to mash this commit history into master-browser-only is going to go badly I think. I think I'll just drop this working tree on top of master-browser-only's source tree, and then you can merge, copy, or make changes on my fork?

chylex commented 10 months ago

If minification is a source of breakages, I might question its value? The userscript and the manual method don't need it, and the bytes it saves are probably going to be eclipsed by what Discord itself uses.

The browser-only version can be used (and originally was only) a bookmarklet, which has size and formatting restrictions.

Copy/pasting the script into dev tools does not necessarily need minification, but it pollutes the dev tools history (makes it impossible to press up/down arrow keys to navigate history) and the console output; it's also the main version I test, so if I minify it, I don't have to test the bookmarklet.

The userscript does not need minification at all.

Trying to mash this commit history into master-browser-only is going to go badly I think. I think I'll just drop this working tree on top of master-browser-only's source tree, and then you can merge, copy, or make changes on my fork?

Doesn't matter to me, but I think if you just open a PR and change the target branch to master-browser-only, it will work. I can then rebase it onto master-browser-only and the unnecessary changes like removing C# files will disappear.

chylex commented 10 months ago

I guess #237 works too :P

will-ca commented 10 months ago

The browser-only version can be used (and originally was only) a bookmarklet, which has size and formatting restrictions.

About that, I noticed the instructions for the bookmarklet mode don't even work on Chrome (or Epiphany). You can still drag the link to the bookmarks bar, but right-clicking for "Bookmark This Link" is a Firefox feature AFAICT. So while it's a neat option, is it really worth increasing the testing burden? Idk.

(makes it impossible to press up/down arrow keys to navigate history)

Annoying, yes. But you just gotta Ctrl+Home and Ctrl+End when you want to navigate the history.

chylex commented 10 months ago

FWIW the instructions are outdated for Firefox too, but you can still bookmark it by dragging the link. The browser-only version only exists for people who need the simplest quickest possible way, and a bookmark is still easier than installing a userscript manager (especially considering the absolute mess of incompatibilities between different browsers and userscript managers), and way easier than going to the website and copy/pasting the script.