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

Various improvements #291

Merged
merged 31 commits into from
Jul 3, 2024
Merged

Various improvements #291

merged 31 commits into from
Jul 3, 2024

Conversation

kearfy
Copy link
Member

@kearfy kearfy commented Jun 29, 2024

Thank you for submitting this pull request! We appreciate you spending the time to work on these changes.

What is the motivation?

We found that the size of the library was getting out of hand and wanted to improve on this. Additionally, with the introduction of JSR, we no longer see the need for this to be a Deno project.

What does this change do?

  • Drops the decimal.js library, as it's unpacked size is 250kb.
  • Introduces a custom CBOR library, optimised for size, speed and our needs.
  • Introduces a custom Duration library, optimised for size, speed and out needs.
  • Introduces a new Uuid wrapper to offer a nicer interface over the uuidv7 library.
  • No longer depends on zod, which was a big contributor to size.
  • No longer depends on semver, which was a big contributor to size. We now compare versions with localeCompare.
  • Adjusted the way that engines work. encodeCbor and decodeCbor are now passed along with the emitter in an EngineContext class, to the engine. This ensures that the engine library uses the same instances of classes that the cbor encoding and decoding depend on.
  • Now uses BiomeJS for faster and further optimised formatting.
  • Now publishes to the JSR registry
  • Fixes certain issues with geometries
  • Fixes an issue which caused the Pinger to be applied to engines outside of ws and wss.
  • Optimises the jsonify method to use custom logic over JSON.stringify() and JSON.parse(), keeping non-surrealql class values intact, and resulting in 100% to 150% faster.
  • Added more tests.
  • We now test across SurrealDB 1.4.2, 1.5.2 and 2.0.0-alpha.3.
  • PreparedQuery instances now partially encode queries and bindings before they are sent over.

What is your testing strategy?

Updated all tests to Bun, and added various new tests. In the future we'd like to expand tests to test on both source and the compiled library, and across Bun, Deno and Node.

Is this related to any issues?

Fixes #284
Supersedes #272

Have you read the Contributing Guidelines?

@kearfy kearfy marked this pull request as draft June 29, 2024 10:52
@kearfy kearfy marked this pull request as ready for review June 29, 2024 17:59
Copy link
Contributor

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

Left some comments
I would extract to multiple PRs for ease of review, one to replace to jsr, one for packaging, one for biome and so on

src/data/types/duration.ts Outdated Show resolved Hide resolved
src/engines/http.ts Outdated Show resolved Hide resolved
src/engines/ws.ts Outdated Show resolved Hide resolved
src/engines/ws.ts Show resolved Hide resolved
src/types.ts Show resolved Hide resolved
src/util/jsonify.ts Show resolved Hide resolved
Copy link
Contributor

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

Left some more comments

Also, I would redo the PR to try to keep the git history as most of the files marked as deleted instead of moved and updated, this has really high value

src/engines/ws.ts Outdated Show resolved Hide resolved
src/engines/ws.ts Outdated Show resolved Hide resolved
src/engines/ws.ts Outdated Show resolved Hide resolved
src/util/versionCheck.ts Outdated Show resolved Hide resolved
tests/integration/tests/auth.test.ts Outdated Show resolved Hide resolved
tests/integration/tests/connection.test.ts Outdated Show resolved Hide resolved
tests/integration/tests/live.test.ts Outdated Show resolved Hide resolved
tests/integration/tests/live.test.ts Outdated Show resolved Hide resolved
tests/integration/tests/querying.test.ts Outdated Show resolved Hide resolved
tests/unit/recordid.test.ts Outdated Show resolved Hide resolved
@kearfy kearfy merged commit a6fd84c into main Jul 3, 2024
8 checks passed
@kearfy kearfy deleted the rework branch July 3, 2024 09:22
@kearfy kearfy mentioned this pull request Jul 3, 2024
1 task
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

Successfully merging this pull request may close these issues.

Bug: MissingNamespaceDatabase: There are no namespace and/or database configured.
2 participants