Skip to content

(fix) remove buggy checks from IsSnowflake + (feature) add JSON conversion#28

Merged
aevitas merged 2 commits into
aevitas:masterfrom
imskyyc:master
Jan 17, 2026
Merged

(fix) remove buggy checks from IsSnowflake + (feature) add JSON conversion#28
aevitas merged 2 commits into
aevitas:masterfrom
imskyyc:master

Conversation

@imskyyc

@imskyyc imskyyc commented Dec 28, 2025

Copy link
Copy Markdown

This is a combination of #27 as well as a new feature for JSON Serialization/Deserialization. Unit tests have been added and all are passing. This also adds package System.Text.Json.

(feature) add JSON conversion
@aevitas

aevitas commented Jan 2, 2026

Copy link
Copy Markdown
Owner

Thanks the pull request, and happy new year! This is effectively #27 and then some - the holidays have stalled that PR for longer than I'd intended. There are some things we should be careful about in this PR though:

  1. We still target .NET 9, but introduce a .NET 10 dependency
  2. We now impose a bunch of dependencies on top of the netstandard2.0 target, because they are implicitly referenced by System.Text.Json
  3. Serializing Flake ID's is tricky

Let's start with the third. In the serializer, the Write is implemented like so:

        public override void Write(Utf8JsonWriter writer, Id value, JsonSerializerOptions options)
        {
            writer.WriteNumberValue(value);
        }

This means that it will end up as a Number object in JSON, which will then be interpreted by a JS runtime as a number type. JavaScript numbers (as outlined further in the README.md of this repo) are 53-bit numbers, as opposed to 64-bit numbers in .NET and other runtimes. If we use this serializer to serialize an ID from .NET, and then use that JSON in a JavaScript application, we'd lose those 11 bits. That could be disastrous for the JS application when it's doing comparisons, for example.

In order to properly serialize the struct, we'd need to write the value as a String. The JS client reads it as a string, does string comparisons, and the crisis would be averted.

The first and second point are less complex fortunately. .NET 8 (the oldest still supported LTS) inherently supports System.Text.Json, and doesn't need the reference. Nor do later versions of .NET. netstandard2.0 does need an external package reference. I think the best way to solve this is so consolidate around .NET 8, with a conditional dependency for the netstandard2.0 target:

  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard2.0'">
    <PackageReference Include="System.Text.Json" Version="8.0.5" />
  </ItemGroup>

And have the package target:

<TargetFrameworks>netstandard2.0;net8.0</TargetFrameworks>

I think combined with the fix to Write, the PR would be a massive improvement to the library, especially for those who serialize IDs directly without referencing the .Value. With these changes, the PR can be merged.

Thanks again!

change Json Parser to parse to strings instead of numbers
@imskyyc

imskyyc commented Jan 3, 2026

Copy link
Copy Markdown
Author

Implemented the above changes with all tests passing! 👍

@imskyyc

imskyyc commented Jan 16, 2026

Copy link
Copy Markdown
Author

Heya! Just bumping this for visibility. Would this be good to merge? Thanks!

@aevitas aevitas merged commit 93a5204 into aevitas:master Jan 17, 2026
1 check passed
@aevitas

aevitas commented Jan 17, 2026

Copy link
Copy Markdown
Owner

I've just released v1.3.1 which includes this change, thanks for the contribution!

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.

2 participants