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

Adds support for serializing bigints. #454

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

MicahZoltu
Copy link
Contributor

@MicahZoltu MicahZoltu commented Feb 5, 2023

I went with serializing bigints as an object with a type and value, but I'm open to other ideas for how to serialize them so they are JSON compatible.

I don't know why I needed to change the type of setInCopy, but the original type definition appears to have been incorrect.

I largely don't understand what most of the properties in parseProps are for, so I just tried to copy other parsers in a way that seemed reasonable. In particular, editable I set to !forceReadonly but this was really just a guess on my part.

I made a few changes to improve the type strictness of JSONValue. If these strictness increases are considered undesirable then everything except the addition of JSONBigint can be removed without causing problems.

Fixes #453

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented Feb 5, 2023

Note: While I added some tests to verify the functionality works, I did not build/install the plugin to verify that #453 is actually fixed.

I went with serializing bigints as an object with a type and value, but I'm open to other ideas for how to serialize them so they are JSON compatible.

I don't know why I needed to change the type of `setInCopy`, but the original type definition appears to have been incorrect.

I largely don't understand what most of the properties in `parseProps` are for, so I just tried to copy other thinsg in a way that seemed reasonable.
In particular, `editable` I set to `!forceReadonly` but this was really just a guess on my part.

I made a few changes to improve the type strictness of `JSONValue`.
If these strictness increases are considered undesirable then everything except the addition of `JSONBigint` can be removed without causing problems.
@MicahZoltu
Copy link
Contributor Author

Fixed PR to actually work. Tested with a dapp and I was able to successfully marshal bigint values from my dapp to the extension, edit them, and marshal them back. I tested regular properties, signals, and hooks and they all appear to work properly both directions.

Feedback definitely appreciated. While everything works and tests all pass, I am a small step up from a monkey smashing on a keyboard when it comes to understanding what I'm doing in this codebase. I think I have figured out enough that this PR should work, but someone more familiar should take a look at it to verify it is all reasonable.

Of particular interest, the place that I'm converting from { type: 'bigint', value: '5' } to 5n when writing values back to the app may not be appropriate. It does appear there is one step higher I could go in the call tree that might work for update-prop calls, but having the check in about the same spot for regular, hook, and signals seemed better.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

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

Awesome, thanks a bunch for the PR 👍

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.

Debug: Could not serialize message.
2 participants