Skip to content

Conversation

@imskyyc
Copy link

@imskyyc imskyyc commented Dec 28, 2025

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
Copy link
Owner

aevitas commented Jan 2, 2026

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
Copy link
Author

imskyyc commented Jan 3, 2026

Implemented the above changes with all tests passing! 👍

@imskyyc
Copy link
Author

imskyyc commented Jan 16, 2026

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
Copy link
Owner

aevitas commented Jan 17, 2026

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