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

fix: add object arrays #274

Merged
merged 1 commit into from
Mar 8, 2024
Merged

Conversation

EwanLyon
Copy link
Contributor

@EwanLyon EwanLyon commented Mar 2, 2024

Previously passing useReplicant an array of objects as the type it would fail even though that is valid JSON. This adds Json[] to the Json type to allow for this.

I also added a type to the test case.

@@ -3,7 +3,7 @@ import { klona as clone } from "klona/json";

type JsonValue = boolean | number | string | null;

type Json = JsonValue | JsonValue[] | { [key: string]: Json };
type Json = JsonValue | JsonValue[] | { [key: string]: Json } | Json[];
Copy link

Choose a reason for hiding this comment

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

May I suggest simply importing and using the Jsonifiable type from the type-fest package? That will cover other edge cases for you like objects with a toJson method, and read-only arrays which both would be rejected by this type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested locally and the Jsonifiable type works. @Hoishin thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Adding package just for this simple type is overkill for me. Edge cases you mentioned don't happen in this case.

@Hoishin Hoishin merged commit be040b3 into nodecg:main Mar 8, 2024
1 check passed
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.

3 participants