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

Replace JSONObject type with JsonDocumentObject #258

Merged

Conversation

amonsosanz
Copy link
Contributor

This PR replaces the type JSONObject with a new type JsonDocumentObject for all the data that can be serialized and parsed as valid JSON, such as queries or credentialSubjects.

The issue with typing serializable documents as JSONObject is that this type includes object as a valid value:

type JSONObject = {
  [x: string]: string | number | boolean | object | Array<object>;
}

object in TypeScript is very broad (basically any value that is not a primitive is an object). This includes functions for example.

This broadness makes it very hard for the clients of this library to parse data that adheres to types using JSONObject, such as the AuthorizationRequestMessage's did_doc.

Copy link
Collaborator

@volodymyr-basiuk volodymyr-basiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@Kolezhniuk
Copy link
Collaborator

I'm ok with changes, but not sure should we rename JSONObject -> JsonDocumentObject. Can't we just preserve JSONObject to prevent others users of sdk change type?

@vmidyllic
Copy link
Collaborator

I'm ok with changes, but not sure should we rename JSONObject -> JsonDocumentObject. Can't we just preserve JSONObject to prevent others users of sdk change type?

as I see JSONObject still exists, so no problem

@vmidyllic vmidyllic merged commit 1e015ef into main Aug 22, 2024
2 checks passed
@vmidyllic vmidyllic deleted the feature/replace-json-object-type-with-json-document-object branch August 22, 2024 14:52
@amonsosanz amonsosanz changed the title Feature: Replace JSONObject type with JsonDocumentObject Replace JSONObject type with JsonDocumentObject Aug 22, 2024
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.

4 participants