Switch to Bun and websockets #106
Closed
Conversation
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
This obviates the need for source-map-support and dotenv dependencies without any code modification. This does not update the docker files since I have no idea how those work.
Seems like node lets you pass in a stringified port, whereas Bun requires a number.
This also means that the message handler will be awaited.
This is largely just renaming "world" to "dimension" and removing unused imports/functions.
This creates a ProtocolHandler interface in server.ts that main.ts implements. Also moves host and port determination to the config, rather than environment.
This also modernises them into classes, and uses symbols instead of strings for matching.
This also fixes up imports and consolidates the deps/*.ts files into a lang.ts file.
This was caused by Bun no longer supporting "RSA_PKCS1_PADDING" or "AES-128-CFB8" ciphers, the former being due to CVE-2023-46809, and the latter probably just being an implementation gap. Either way, since the protocol needs to be changed anyway, might as well switch to websockets and let TLS optionally protect the connection. Okx be having an aneurysm rn.
The fact that `getChunkTimestamps` would create an entire disposable shadow-table is wonder to behold. Also fixed a bug caused by my naivety Bun's sqlite type could just be cast to kysely's since they're both based off of better-sqlite3. Given that there weren't any errors, my guess is that additional parameters were just being silently swallowed? There's currently no migration for pre-existing MapSync databases.
Tested this out and where-in took around 2.9 seconds using an example 4.3GB database, whereas the where-and-or-chain version took ***44.2*** seconds.
This came from removing zod-validation-error as a dependency in d429342 since Zod v4 provides its own error pretty-printer. But removing it apparently didn't remove the files from my node_modules. Alas.
Added the "gen_" prefix to generated columns to convey that these are basically readonly values. Also, being able to make generated columns from generated columns is amazing. And switching setup() and storeChunkData() to transactions.
This'll mean that any existing MapSync database (like the example 4.6GB database) will be migrated to have generated columns.
Member
|
@Huskydog9988 can you review this? I'm OK with the changes, just don't have time to go over the code. @Protonull do you want to fix the CI? https://github.blog/changelog/2024-04-16-deprecation-notice-v3-of-the-artifact-actions/ |
Closed
Huskydog9988
previously approved these changes
Jun 14, 2025
Contributor
Huskydog9988
left a comment
Huskydog9988
left a comment
There was a problem hiding this comment.
my brain was pretty fried going into this, so i didn't dig that deep, but lgtm from everything I skimmed
Contributor
|
Can you update the versions of the stuff we use in github actions while you have this pr open? Rn apparently the build is failing because the checkout action is deprecated for instance. |
This *should* fix the workflows. The build-* workflows no longer publish a new release, instead there's a new release workflow that builds the mod (and later the server, assuming that's wanted). Also updated the Dockerfile with direction from Husky, but some additional work is needed.
Turns out Bun doesn't like it if you try to run tests and there aren't any tests yet. Easy fix, we should probably have a test for migrations anyway.
Converts the database connection into an instanced-class. This is to avoid the creation of the "DATA_FOLDER" within the database test since tests should probably be pure.
The websocket server does not provide an error, but rather a close code and reason.
|
|
||
| #Mount your FS or volume or whatnot to this folder | ||
| RUN mkdir /data | ||
| # TODO: Fix env override of config data |
Contributor
There was a problem hiding this comment.
did you ever fix the config thing we talked about? i forget the specifics but I remember there being some issue with it in docker.
Comment on lines
+10
to
+12
| "check:types": "bunx --bun tsc --noEmit --checkJs", | ||
| "check:style": "bunx --bun prettier --check .", | ||
| "format": "bunx --bun prettier -w .", |
Contributor
There was a problem hiding this comment.
why are you using bunx here? these packages are installed locally already for development
Comment on lines
+70
to
+72
| export function inlineThrow<T>(error: any): T { | ||
| throw error; | ||
| } |
Contributor
There was a problem hiding this comment.
two things here,
- what is the purpose of this function, and can its purpose be accomplished without using a stage 2 proposal?
- the generic is unused, but it seems like the error is meant to be typed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This was initially just meant as a low-key switch to Bun but it has since gotten away from me. What precipitated the switch was the following features:
Native TypeScript support, which obviates the TypeScript to JavaScript compile-step as well as source mapping.
Native SQLite support, as while nodejs v22 LTS has an SQLite API, it remains 1.1 experimental, even in the latest version.
ArrayBufferSink. Need I say more?
Native compile-to-executable support, which is something MapSync has considered for a while via pkg but I don't think it ever came to fruition?
Native websocket support, which came in clutch with d429342, especially since the API is far better than ws, which had been discussed previously with consider switching to websockets #43 and 1.19.2 Version! #103 (comment). Websockets also add contextual close reasons, so having the connection not auto-reconnect if the user was kicked (rather than disconnecting) is possible without any additions to the protocol.
This PR also offers:
Stateful handshaking and authentication. The 'API' for requiring auth / testing for online auth leaves something to be desired, but it's far better than testing whether ciphers exist or whether the user's uuid is truthy.
Generated columns are amazing. Just discovered this only last month but it should drastically improve performance and response time since the query is not needing to generate and populate a table on demand only to immediately discard it (and this is per [re]connection). There's also a switch from using "in": not sure to what degree this will help, but I assume that integer comparison is faster than string comparison.
Zero encryption handling. The switch to websockets basically defers the issue of encryption to a reverse proxy like Caddy, which offers free and automatic TLS certificates via Let's Encrypt.
Issues this PR resolves: