github-starred/actual
2
0
Fork 0
mirror of https://github.com/actualbudget/actual.git synced 2026-05-06 07:01:45 -05:00
Code Issues 1.5k Packages Projects Releases 54 Wiki Activity

[GH-ISSUE #86] Recommended review order for my PRs #49218

New Issue
Closed
opened 2026-04-30 10:26:34 -05:00 by GiteaMirror · 2 comments
GiteaMirror commented 2026-04-30 10:26:34 -05:00
Owner
Copy Link

Originally created by @TomAFrench on GitHub (Jun 3, 2022).
Original GitHub issue: https://github.com/actualbudget/actual/issues/86

I'm leaving this comment as it is for the record but I've made a new list as a fresh comment below.

As mentioned in Discord, here's general rundown of my PRs. I've arranged them in an order which seems logical in terms of priorities and should minimise merge conflicts. I haven't considered how any of the other PRs should factor into this.

1. Trivial changes

Mostly stuff which doesn't touch the core code at all but tidies up the repo:

#46 reorders the dependencies in the package.json files into alphabetical order, yarn does this automatically when installing new dependencies so this means other PRs which do this have less noisy diffs.
#54 updates the links in the importer packages so that they point at this repo.
#47 adds a missing comma to make jest.config.js valid JS.
#48 cleans up some files which don't seem to be used and were committed by mistake.
#69 enforces the sorting of entries in data-file-index.txt as I get a different order to what's committed.
#81 removes some yarn scripts which point at nothing so always fail.
#80 creates yarn scripts for desktop-electron package to simplify bin/package (useful if we upgrade to yarn v3)
#96 whitespace changes to the CI config

2. Documentation

No significant code changes, just writing READMEs:

#65 adds documentation for how to generate the protobuf.

3. CI

3.1 Laying groundwork

Before we can set up CI we need to do a couple of fixes so that we can install dependencies in CI without it failing:

#53 Removes an unused patch which causes yarn install to fail in CI.
#55 Locks the dependency versions of patched packages so that we always install the patched version (a mismatch causes yarn install to fail in CI)

  • Alternatively we can merge #89 to update the patches to match the versions currently being installed.

3.2 Checking builds succeed

I've got some branches which work on migrating us up to Webpack 5 and swc-loader but I think we need some CI to test all the different builds we do first:

#72 Updates the shebang in some scripts so it uses bash instead of sh. We're using bash features in these scripts so they're failing on my machine and CI with sh.
#73 Adds a github action CI to check that we can build all the various outputs on any PR.

3.3 Linter checks

#70 Adds a github action CI to check for any linter errors in loot-core

4 Fixes

#59 Fixes the broken Timestamp test suite and updates the Timestamp implementation to enforce the proper validity checks.
#102 Fixes a broken test by scaling up some values so they're all integers.

5 Reorganising code

These shouldn't change functionality at all, I'm mostly just moving code around so that it's easier to read/import:

#64 Group timestamp.js and merkle.js into a crdt directory as they're intrinsically linked.
#68 Create a single entrypoint for importing AQL code rather than reaching into multiple different files in the aql directory
#83 Breaks the functions contained in db/index up into multiple files based on which sort of data they're working on.

6 Migration to typescript

These PRs are going to muck with CI and have a bunch of merge conflicts with the PRs above. I can rebase these on top of master once we get closer to the point of merging.

6.1 Migrate to Yarn v3

This isn't necessarily a hard requirement for migrating to typescript but it definitely simplifies it. Running tests in a monorepo is much simpler in yarn v3 so I'd strongly recommend making this change so we can get rid of some of the hacks around testing.

#50 Upgrades from yarn v1 to yarn v3

6.1 Migrate tests

Before we can migrate the code, we need to migrate how we run the tests. Otherwise we end up trying to import Typescript code and then jest doesn't know what to do with it. If we swap out babel-jest for ts-jest the tests run fine without any modification and will still work once we add typescript to the mix.

I'm not making a PR for this just yet as the diff is so noisy with required prerequisite changes that it's not really worth looking at yet. The relevant commit however is 28056eae68

6.2 Migrate loot-core

At this point we're now ready to start using Typescript code so we can add TS support to Webpack. This PR currently uses yarn v1 and will need to be rebased on top of #50 at some point before we merge.

The code is still 99.99% javascript after this but this allows us to progressively migrate files over time:

#84 Adds support for Webpack to use .ts files in the build and swaps out linter for a TS compatible one.

Originally created by @TomAFrench on GitHub (Jun 3, 2022). Original GitHub issue: https://github.com/actualbudget/actual/issues/86 I'm leaving this comment as it is for the record but I've made a new list as a fresh comment below. ---- As mentioned in Discord, here's general rundown of my PRs. I've arranged them in an order which seems logical in terms of priorities and should minimise merge conflicts. I haven't considered how any of the other PRs should factor into this. ## 1. Trivial changes Mostly stuff which doesn't touch the core code at all but tidies up the repo: #46 reorders the dependencies in the package.json files into alphabetical order, yarn does this automatically when installing new dependencies so this means other PRs which do this have less noisy diffs. #54 updates the links in the importer packages so that they point at this repo. #47 adds a missing comma to make `jest.config.js` valid JS. #48 cleans up some files which don't seem to be used and were committed by mistake. #69 enforces the sorting of entries in `data-file-index.txt` as I get a different order to what's committed. #81 removes some yarn scripts which point at nothing so always fail. #80 creates yarn scripts for `desktop-electron` package to simplify `bin/package` (useful if we upgrade to yarn v3) #96 whitespace changes to the CI config ## 2. Documentation No significant code changes, just writing READMEs: #65 adds documentation for how to generate the protobuf. ## 3. CI ### 3.1 Laying groundwork Before we can set up CI we need to do a couple of fixes so that we can install dependencies in CI without it failing: #53 Removes an unused patch which causes `yarn install` to fail in CI. #55 Locks the dependency versions of patched packages so that we always install the patched version (a mismatch causes `yarn install` to fail in CI) - Alternatively we can merge #89 to update the patches to match the versions currently being installed. ### 3.2 Checking builds succeed I've got some branches which work on migrating us up to [Webpack 5 and `swc-loader`](https://github.com/TomAFrench/actual/pull/3) but I think we need some CI to test all the different builds we do first: #72 Updates the shebang in some scripts so it uses `bash` instead of `sh`. We're using bash features in these scripts so they're failing on my machine and CI with `sh`. #73 Adds a github action CI to check that we can build all the various outputs on any PR. ### 3.3 Linter checks #70 Adds a github action CI to check for any linter errors in `loot-core` ## 4 Fixes #59 Fixes the broken Timestamp test suite and updates the `Timestamp` implementation to enforce the proper validity checks. #102 Fixes a broken test by scaling up some values so they're all integers. ## 5 Reorganising code These shouldn't change functionality at all, I'm mostly just moving code around so that it's easier to read/import: #64 Group `timestamp.js` and `merkle.js` into a `crdt` directory as they're intrinsically linked. #68 Create a single entrypoint for importing AQL code rather than reaching into multiple different files in the `aql` directory #83 Breaks the functions contained in `db/index` up into multiple files based on which sort of data they're working on. ## 6 Migration to typescript These PRs are going to muck with CI and have a bunch of merge conflicts with the PRs above. I can rebase these on top of master once we get closer to the point of merging. ### 6.1 Migrate to Yarn v3 This isn't necessarily a hard requirement for migrating to typescript but it definitely simplifies it. Running tests in a monorepo is much simpler in yarn v3 so I'd strongly recommend making this change so we can get rid of some of the hacks around testing. #50 Upgrades from yarn v1 to yarn v3 ### 6.1 Migrate tests Before we can migrate the code, we need to migrate how we run the tests. Otherwise we end up trying to import Typescript code and then jest doesn't know what to do with it. If we swap out `babel-jest` for `ts-jest` the tests run fine without any modification and will still work once we add typescript to the mix. I'm not making a PR for this just yet as the diff is so noisy with required prerequisite changes that it's not really worth looking at yet. The relevant commit however is https://github.com/TomAFrench/actual/commit/28056eae687850e2b44fa3248bb16784005acd3a ### 6.2 Migrate loot-core At this point we're now ready to start using Typescript code so we can add TS support to Webpack. This PR currently uses yarn v1 and will need to be rebased on top of #50 at some point before we merge. The code is still 99.99% javascript after this but this allows us to progressively migrate files over time: #84 Adds support for Webpack to use .ts files in the build and swaps out linter for a TS compatible one.
GiteaMirror commented 2026-04-30 10:26:36 -05:00
Author
Owner
Copy Link

@TomAFrench commented on GitHub (Jun 30, 2022):

1 Useful fixes/features

These aren't my PRs but I think they should be prioritised

#117 Improves CSV parsing to handle dates without a delimiter.
#112 Display client + server versions on settings page.

2 Simplifying dependencies

#94 Replace the currency-formatter dependency with Intl.NumberFormat
#125 Replace node-libofx with ofx-js

3 Reorganising code

These shouldn't change functionality at all, I'm mostly just moving code around so that it's easier to read/import:

#52 Remove imports which aren't used at all.
#64 Group timestamp.js and merkle.js into a crdt directory as they're intrinsically linked.
#68 Create a single entrypoint for importing AQL code rather than reaching into multiple different files in the aql directory
#83 Breaks the functions contained in db/index up into multiple files based on which sort of data they're working on.

4 Migration to SWC

SWC doesn't support the version of webpack we're using so before we can attempt to migrate to that we need to update to the new version of webpack. Once that's done it's relatively simple to move to SWC.

#108 Migrate from webpack v4 to v5
#109 Migrate to SWC

<!-- gh-comment-id:1171742457 --> @TomAFrench commented on GitHub (Jun 30, 2022): ## 1 Useful fixes/features These aren't my PRs but I think they should be prioritised #117 Improves CSV parsing to handle dates without a delimiter. #112 Display client + server versions on settings page. ## 2 Simplifying dependencies #94 Replace the currency-formatter dependency with Intl.NumberFormat #125 Replace `node-libofx` with `ofx-js` ## 3 Reorganising code These shouldn't change functionality at all, I'm mostly just moving code around so that it's easier to read/import: #52 Remove imports which aren't used at all. #64 Group `timestamp.js` and `merkle.js` into a `crdt` directory as they're intrinsically linked. #68 Create a single entrypoint for importing AQL code rather than reaching into multiple different files in the `aql` directory #83 Breaks the functions contained in `db/index` up into multiple files based on which sort of data they're working on. ## 4 Migration to SWC SWC doesn't support the version of webpack we're using so before we can attempt to migrate to that we need to update to the new version of webpack. Once that's done it's relatively simple to move to SWC. #108 Migrate from webpack v4 to v5 #109 Migrate to SWC
GiteaMirror commented 2026-04-30 10:26:36 -05:00
Author
Owner
Copy Link

@TomAFrench commented on GitHub (Jul 24, 2022):

Closing this as it's out of date and this isn't a good way of communicating about PR status.

<!-- gh-comment-id:1193364047 --> @TomAFrench commented on GitHub (Jul 24, 2022): Closing this as it's out of date and this isn't a good way of communicating about PR status.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
AI generated
API
bank sync
budgeting
bug
can’t replicate
dependencies
docker
documentation
electron
experimental feature
feature
feedback
goal templates
good first issue
help wanted
importers
maintenance
needs info
needs testing
needs triage
needs votes
openid
payees
pull-request
Mirrored from GitHub Pull Request
 regression
reports
responsive
rules
schedules
server
✨ merged
split transactions
tech debt
theme
transaction import
transaction reconciliation
transactions
translations
upstream
user interface
✅ approved
wontfix
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
GiteaMirror ninjasurge
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/actual#49218
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?