Bring in other contributors' code from dev and beyond, add a CI build, add structure for automated tests, simplify package management tools - #240
Open
airbreather wants to merge 29 commits into
Conversation
…ccidental overriding of previous hints. (ThePhar#223)
- Added `.editorconfig` to ensure consistent code formatting. - Introduced and configured GitHub Actions for CI. - Migrated helper / config scripts from `.mjs` to `.ts`. - Updated and streamlined dependencies and `tsconfig.json`, using just Bun for everything we can. - Refactored package scripts in `package.json` and updated package metadata. - Implemented very rudimentary tests to get us started here. - `index.test.ts` just imports the entire top-level module so that all the static code gets marked as covered. - `utils.test.ts` gets a foot in the door by testing a trivial utility function so we can see it run *something*. - `connect.test.ts` is an integration test that actually connects to a server and does something. - Added `Containerfile` for integration testing of a simulated server environment. - Removed `.npmignore` and exclusions in `tsconfig.json`- we can achieve the same by only listing **inclusions**.
Conflicts: README.md package.json pnpm-lock.yaml src/classes/Hint.ts src/classes/managers/DataPackageManager.ts src/classes/managers/ItemsManager.ts tsconfig.json
Conflicts: src/api/packets/client/UpdateHint.ts src/api/types.ts src/classes/Client.ts src/classes/Hint.ts src/classes/managers/DataPackageManager.ts src/classes/managers/MessageManager.ts tsconfig.json
… cases in here than either ThePhar#236 or 6047b13 handled. Let's make this the first interesting use of the integration tests.
…sion test was thought to be something I'd just tack onto the end of the smoke test.
…als: 1. Remove redundant declarations. 2. Move ignores to their own section to make it more obvious that they're applied globally, not scoped. 3. Explicitly identify the file extensions that are affected by each sub-configuration. 4. Configure the stylistic and jsdoc plugins using their provided factory functions. 5. As much as possible, express "that config, but ..." using just `extends` for consistency and clarity. 6. Bring in a plugin for `package.json`. I can discern no intention behind the order of properties in this file. 7. Pull in jiti, which is needed for ESLint to run on things other than Bun or Deno. In other words, `npx eslint` works again, in case that's your thing. The `eslint` script should now run as-is without any further arguments. MANY files now yield failures that were not caught by earlier attempts, with more than 300 errors about missing semicolons. Before applying the rules to address these failures, the next commit on this branch will adjust the configuration to account for apparent preferences that conflict with the defaults brought in by doing things this way.
…perfectly match the existing code base's style. Besides package.json rules introduced on this branch, the only rules that now fail with this config are: 1. `@typescript-eslint/array-type`: there's a clear, strong preference (122 vs 3) for the default configuration of this rule. the 3 violations should just be fixed. 2. `@typescript-eslint/consistent-generic-constructors`: there are two ways to configure this rule while keeping it on. neither configuration is universal, and there are very few isolated places either way we go. keep default. 3. `@typescript-eslint/prefer-for-of`: perhaps unsurprisingly, the norm is to use `for..of` loops instead of index-based. of the 4 `for` loops in this project, 1 is a "repeat n times" style (good) 2 are just `for..of` loops in disguise, and the remaining 1 is also a disguised `for..of` loop but doesn't get caught by ESLint. 4. `@typescript-eslint/prefer-nullish-coalescing`: there's exactly one violation of this and a couple of places where `??=` is used identically.
… previous two commits landed: 1. one of the `for` --> `for..of` fixes didn't really go far enough: the parent was still a `for..of` loop that wanted to break free. filling each slot by indexing beyond the end of the array (as opposed to just calling `push`) was weird enough for me to pull the trigger on rewriting it as the two-dimensional `map` that it always wanted to be. 2. the auto-fix in `package.json` made an interesting choice of what whitespace to use for indenting the `exports` property's value.
…sting violations that require breaking changes to fix. Also suppress the violation in `DeathLinkManager.ts`, even though it would not be a breaking change to fix it. A fix would ultimately wind up either suppressing something else or writing some unusual type guard stuff that's more trouble than it's worth at this exact moment.
See the individual commits included for details of what's changing.
airbreather
marked this pull request as ready for review
April 23, 2026 01:32
airbreather
marked this pull request as draft
April 23, 2026 01:42
…anges. I had originally intended to do this all out of my own fork repo, but Phar is back at least for now, so this can stay. Also, make sure the docs can build.
airbreather
marked this pull request as ready for review
April 23, 2026 01:45
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Special request when merging
Because there have been multiple discrete changes by multiple different contributors at multiple different times, spanning PRs targeting branches on multiple different repos, I would like to request that this ONLY be merged using the "Create a merge commit" option (NOT squash, NOT rebase):
Now to the main description
Background
Over the past ~year, a couple of us have been working on our own forks of the project as new requirements emerged. I eventually published a prerelease package just to unblock my application's need for hint functionality. After seeing your message earlier this year, I privately decided that once I got a 1.x version of Autopelago released, then I would try to see what I could do about making my fork more than just "@DrAwesome4333's patches, but packaged and released on npm", and take on maintenance of this package on my fork.
Coincidentally, 2.1.0 was released right as I was getting to the end of the first major step forward. If you want to keep your version going, then I'm more than happy to contribute PRs.
This PR
This one's quite big because it's a combination of multiple things. If you want to review them, I'd recommend looking at it the way that I did, i.e., one item at a time as they were merged into my own
mainbranch. In each case, I made sure that the PR description explains what that specific PR is doing, but I'll TL;DR each one below.OTHER THAN THE FIRST ONE IN THIS LIST, the other three PRs were carefully merged with a regular merge commit rather than a squash. 2 and 3 were because the individual commits really are part of the history, and 4 is that way because these kinds of changes can make the history irrevocably look REALLY chaotic if you're not deliberate about exactly how it's done.
airbreather#1 makes no changes to the actual meat of the project, only the scaffolding and structure around it. Don't worry about the
package.jsonchanges in this PR that make it say "airbreather", that's getting reverted in a later standalone commit on this branch.TL;DR there are now SOME automated tests (#14), replace pnpm with Bun since we already use that for other things, and run stuff in CI including a basic integration test with a real Archipelago server.
bun pm packis the magic now. It runs theprepackscript first (notdocs, see later).airbreather#2 - the
devbranch had some unique stuff that I guess was intended formainduring 2.1.0. There were some changes ondevthat were duplicated onmain, as well as some things unique todev.airbreather#3 pulls in the rest of @DrAwesome4333's changes that didn't make it to the
devbranch for one reason or another.This included a different fix for #232 which (as I've been corrected) is actually more complete than what made it into 2.1.0. While I was writing the automated tests for it, I realized that it was still incomplete, so I extended it to cover those last ones (specifically, a bare
'!admin'and the variants of'!admin /option server_password').Loose commit dacbc33 came next, mentioning only for completeness.
airbreather#4 is the last big thing I feel comfortable doing all at once now that I know that I'm going to be making this PR, since it's already way too much to ask someone to look at. It addresses some inconsistencies / oversights / no-longer-best-practices in the ESLint config.
EACH COMMIT in this PR contributes a different specific thing to the solution and was designed with the idea in mind that someone looking back through history would encounter them individually.
I admittedly thought, when starting this work, that the status quo was worse off than it actually was. The majority of my initial concern was just seeing more than half the files getting touched by the auto-fix. But with a bit of smart configuration, every change makes sense now. There were still some fairly impactful things here.
stylisticTypeCheckedconfig, in addition to thestylisticfor regular JS that probably should always go together with it for a project like this. most of these rules are truly just a matter of preference, so I configured them as needed to avoid overwhelming churn. the two exceptions were addressed here, and I strongly believe that these defaults should be followed for new code, especially when you have use cases for broad types likeJSONRecordandEventBasedManager.package.jsonmatch the de facto standard style, since I kept spending way too much time trying to figure out where to put new properties like"sideEffects": false.Loose commit c58e800 rounds it out, now that this PR is happening.
Next steps
Back here, I said that I wanted to check that it works with lower ES2015 targets before pushing forward with this. I've changed my mind on that - I can send this PR and just trust that TypeScript isn't lying or broken.
I've publicly mentioned a bunch of different incompatible "next" ideas in various places, so take this with a grain of salt... having said that, I think #210 is the natural next step since I've already done auto-publishing to GitHub Pages for a different project.