Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: more TypeScript migration #499

Draft
wants to merge 27 commits into
base: main
Choose a base branch
from
Draft

Conversation

ROpdebee
Copy link
Owner

@ROpdebee ROpdebee commented Jun 16, 2022

I'm not refactoring these a lot (yet), just migrating to TypeScript so we can benefit from all the benefits. Also removing jQuery where possible (mainly because I've already removed it from the dependencies and don't want to re-add it). Migrating to JSX where it makes sense too. And fixing linter warnings, of course.

Coverage is probably going to take a nosedive but it was a lie anyway. I've tested the new versions manually to ensure they still work.

  • mb_blind_votes.user.js
  • mb_bulk_copy_work_codes.user.js
  • mb_qol_inline_recording_tracks.user.js
  • mb_qol_seed_recording_disambiguation.user.js
  • mb_qol_select_all_update_recordings.user.js
  • mb_supercharged_caa_edits.user.js
  • mb_validate_work_codes.user.js
  • Update old scripts so they auto-update to the rewritten version.
  • Update links in README.

I'll probably merge validate work codes into bulk copy work codes and rename the latter too.

@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #499 (7014446) into main (d1813bb) will decrease coverage by 24.14%.
The diff coverage is 4.37%.

@@             Coverage Diff             @@
##             main     #499       +/-   ##
===========================================
- Coverage   98.48%   74.34%   -24.15%     
===========================================
  Files          58       63        +5     
  Lines        1385     1863      +478     
  Branches      220      293       +73     
===========================================
+ Hits         1364     1385       +21     
- Misses         13      462      +449     
- Partials        8       16        +8     
Impacted Files Coverage Δ
src/lib/MB/URLs.ts 100.00% <ø> (ø)
src/lib/util/array.ts 70.83% <0.00%> (-12.50%) ⬇️
src/lib/util/dom.ts 70.58% <0.00%> (-18.31%) ⬇️
src/lib/util/xhr.ts 100.00% <ø> (ø)
src/mb_blind_votes/index.ts 0.00% <0.00%> (ø)
...mb_enhanced_cover_art_uploads/providers/archive.ts 100.00% <ø> (ø)
src/mb_supercharged_caa_edits/constants.ts 0.00% <0.00%> (ø)
src/mb_work_code_toolbox/identifiers.ts 0.00% <0.00%> (ø)
src/mb_work_code_toolbox/index.ts 0.00% <0.00%> (ø)
src/mb_work_code_toolbox/validate.ts 0.00% <0.00%> (ø)
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@kellnerd
Copy link
Collaborator

Just wanted to remember you, that there already is https://github.com/ROpdebee/mb-userscripts/tree/wip-blind-edits-rewrite

@ROpdebee
Copy link
Owner Author

ROpdebee commented Jun 16, 2022

Yeah, that's a pretty big rewrite though. I'd like to get back to that eventually, but here I'm just focusing on refactoring them enough to get them to pass typechecking, linting, building and eventually deploying.

Also, I don't like bulk copy work codes' implementation. Very messy. 😅

Edit Oooh boy, Supercharged is 1000 lines of pain 😢 Less pain than the work codes, but still pain. Especially that jQuery UI widget.

I didn't remove jQuery in the artwork comparison dialog, because it
uses jquery-ui and because I'm afraid I'll break something. More jQuery
removal is probably something we should do later, but I've decided against
doing it in this migration pass. I also updated all dependencies.
@ROpdebee ROpdebee force-pushed the typescript-migration branch from 76e361f to f419257 Compare June 17, 2022 17:20
@ROpdebee
Copy link
Owner Author

/deploy-preview

@github-actions
Copy link

github-actions bot commented Jun 17, 2022

This PR changes 9 built userscript(s):

See all changes

github-actions bot added a commit that referenced this pull request Jun 17, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Jun 17, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Jun 17, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Jun 17, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Jun 17, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Jun 17, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Jun 17, 2022
refactor: more TypeScript migration (#499)
@ROpdebee ROpdebee marked this pull request as ready for review June 17, 2022 19:31
@ROpdebee
Copy link
Owner Author

I've superficially tested all of these, and tested Supercharged in a bit more depth because the changes were a bit more complicated there. Seems to work fine on my end, but I suggest leaving this open for a while longer to make sure there are no regressions. I'll probably post in the forums too to ask people to try out the previews and report any regressions.

@ROpdebee
Copy link
Owner Author

/deploy-preview with latest fixes

github-actions bot added a commit that referenced this pull request Jun 19, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Jun 19, 2022
refactor: more TypeScript migration (#499)
@ROpdebee ROpdebee force-pushed the typescript-migration branch from e43f8e6 to 5cfe324 Compare June 19, 2022 23:49
document.querySelectorAll('iframe').forEach((iframe) => {
if (!iframe.contentWindow) return;

onWindowLoaded(() => {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How the hell did this ever work in the rewritten version? We're asynchronously adding stuff to an array but then consuming the array synchronously 🤦

@ROpdebee
Copy link
Owner Author

ROpdebee commented Jul 4, 2022

Noting for later: Supercharged is missing some separator on release events, c227924 wasn't a good fix.

2005-05-23 (GB)2005-06-06 (DE)

pretty brute force, needs to be checked again in the future. but
chances are i'll just drop all of these commits and split the whole
PR anyway. Just need to do some quick fixes.
@ROpdebee
Copy link
Owner Author

ROpdebee commented Aug 8, 2022

/deploy-preview let's see if this still works? We need to regenerate CAA Dimensions because the preview version was probably updated to a more recent main release while Supercharged is still expecting the new interface (it's very messy currently).

github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
github-actions bot added a commit that referenced this pull request Aug 8, 2022
refactor: more TypeScript migration (#499)
@ROpdebee ROpdebee marked this pull request as draft April 23, 2023 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants