Skip to content

Replace the build system, closes #110, #111 - #112

Merged
johnridesabike merged 7 commits into
masterfrom
110-replace-the-build-system
Dec 8, 2025
Merged

Replace the build system, closes #110, #111#112
johnridesabike merged 7 commits into
masterfrom
110-replace-the-build-system

Conversation

@PascalHonegger

Copy link
Copy Markdown
Collaborator

No description provided.

@PascalHonegger PascalHonegger added enhancement New feature or request dependencies Pull requests that update a dependency file labels Nov 16, 2025
This was linked to issues Nov 16, 2025
@PascalHonegger PascalHonegger changed the title Replace the build system, closes #110 and #111 Replace the build system, closes #110, #111 Nov 16, 2025
@PascalHonegger
PascalHonegger force-pushed the 110-replace-the-build-system branch from e640cc6 to 4e1192f Compare November 16, 2025 16:56
@PascalHonegger
PascalHonegger force-pushed the 110-replace-the-build-system branch from 4e1192f to 7eacd53 Compare November 16, 2025 16:59
@PascalHonegger
PascalHonegger force-pushed the 110-replace-the-build-system branch from 7eacd53 to 80fe7ec Compare November 16, 2025 17:03
@PascalHonegger PascalHonegger self-assigned this Nov 16, 2025
@johnridesabike

Copy link
Copy Markdown
Owner

Thanks! Vite definitely looks like an improvement over my original custom system.

Regarding the tests, I fetched this branch and played with a little on my machine, and I'm still a puzzled by the failing tests. My assumption right now is that there's something about the Jest mocks that isn't working. The mocks achieve a couple of basic things: replace the asynchronous localstorage code with synchronous code, and preload the test data for tournaments and players. Since the "Loading..." text is appearing in test snapshots now, it doesn't seem like either of those things is happening.

I'll look at it closer once I get a chance. If the mocks are indeed the problem, I'm partially wondering if it's worth keeping them, although then we'd still have to solve the headache of dealing with asynchronous localstorage while testing.

@johnridesabike

Copy link
Copy Markdown
Owner

I think I may have found the root issue, although I haven't fixed anything yet to confirm it. It seems that the current vi.mock works differently than jest.mock that was used previously, which I assume is the problem.

Right now, I'm feeling more inclined to avoid mocks entirely by passing the data and DB implementation explicitly. That would likely mean adding parameters to a lot of components though, which would introduce a bigger diff in the source code (which wouldn't necessarily be a bad thing, once it's complete). We can see how well it works or if there's an easier solution.

@johnridesabike

Copy link
Copy Markdown
Owner

It looks like this broke the localforage bindings, so I just pushed a commit to fix that. I'm still working on fixing the tests.

@PascalHonegger

Copy link
Copy Markdown
Collaborator Author

It looks like this broke the localforage bindings, so I just pushed a commit to fix that. I'm still working on fixing the tests.

Yes localforage is kinda a problem because they don't support ES6 natively, once you're done or think I can assist just hit me up 👌

@johnridesabike

Copy link
Copy Markdown
Owner

I just pushed another commit that should fix the tests. A few notes for posterity:

  • After returning to these LocalForage bindings several years later, I think the original ones were way too ambitious. I trimmed them down significantly.
  • The previous mocks are now replaced with just one LocalForage replacement that lives inside the Db module. The module checks an __IS_TEST__ variable to decide which storage implementation to use.
  • The tests are all now async-friendly.
  • Before this implementation, I tried using vitest-indexeddb and localforage-memoryStorageDriver packages to shim the storage, but neither one seemed to work no matter how I invoked them. (LocalForage always complained a compatible driver wasn't available.) Maybe I was doing something wrong, but either way I think the current implementation is superior due to its simplicity.

I'm going to come back and review it again when I get the time (that may be after the holiday here in the US) but this hopefully should be the start of getting things back on track now.

@johnridesabike

Copy link
Copy Markdown
Owner

It looks like GitHub actions failures might be due to this issue: jihchi/vite-plugin-rescript#231

@PascalHonegger Feel free to fix that if you're able to. I'll take a closer look once I have a chance, but that probably won't be this week.

@johnridesabike
johnridesabike force-pushed the 110-replace-the-build-system branch from 046b7d0 to 38bd489 Compare December 5, 2025 13:43

@johnridesabike johnridesabike left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I've fixed the tests and cleaned up my last few commits. Right now, I think these changes all look good. Unless one of us notices anything else we need to fix, I'm happy to merge this soon.

Also simplify the localforage code
@johnridesabike
johnridesabike force-pushed the 110-replace-the-build-system branch from 38bd489 to e4ff2e5 Compare December 6, 2025 03:21
@johnridesabike
johnridesabike marked this pull request as ready for review December 6, 2025 21:44
@johnridesabike
johnridesabike merged commit 1cd5aa0 into master Dec 8, 2025
5 checks passed
@PascalHonegger
PascalHonegger deleted the 110-replace-the-build-system branch December 8, 2025 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The test suite is broken Replace the build system

2 participants