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

Recommended review order for my PRs #86

Closed
TomAFrench opened this issue Jun 3, 2022 · 2 comments
Closed

Recommended review order for my PRs #86

TomAFrench opened this issue Jun 3, 2022 · 2 comments

Comments

@TomAFrench
Copy link
Contributor

TomAFrench commented Jun 3, 2022

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 Update patches #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 TomAFrench@28056ea

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.

@TomAFrench
Copy link
Contributor Author

TomAFrench commented 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

@TomAFrench
Copy link
Contributor Author

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

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

No branches or pull requests

1 participant