Skip to content

feat: Enhance VariableEditor to support BigNumber type handling #55

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

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

Suntgr
Copy link
Contributor

@Suntgr Suntgr commented Jan 22, 2025

  • Added bigNumber property to ReactJsonView and VariableEditor components.
  • Introduced JsonBigNumber component for rendering BigNumber values.
  • Updated parseInput and stringifyVariable functions to handle BigNumber parsing and stringification.
  • Modified toType utility function to recognize BigNumber instances.
  • Updated README.md to document the new bigNumber property and its usage.

This update improves the precision handling of numbers in the application.

Copy link

vercel bot commented Jan 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-json-view ❌ Failed (Inspect) Feb 12, 2025 8:49am

Copy link
Member

@Kikobeats Kikobeats left a comment

Choose a reason for hiding this comment

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

Added some comments; Additionally, you should to add it into the dev-server too
https://github.com./microlinkhq/react-json-view/blob/main/dev-server/src/index.js#L199

@Suntgr
Copy link
Contributor Author

Suntgr commented Feb 10, 2025

Added some comments; Additionally, you should to add it into the dev-server too https://github.com./microlinkhq/react-json-view/blob/main/dev-server/src/index.js#L199

I've completed the requested changes. Is there anything else that needs to be added?

@Kikobeats
Copy link
Member

@Suntgr sorry I just reviewed right now, It seems great however test are broken in the main repo and that is blocking this.

Fixing tests should be addressed first, appreciate if you can open a new PR for that.

@Kikobeats Kikobeats self-requested a review February 10, 2025 10:03

if (parent_type === 'array_group' && index_offset) {
variable.name = parseInt(variable.name) + index_offset
}
if (!variables.hasOwnProperty(name)) {
if (!Object.prototype.hasOwnProperty.call(variables, name)) {
Copy link
Member

Choose a reason for hiding this comment

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

@Suntgr what's the motivation for this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The variables might be a special object (like a json-bigint instance) that either doesn't inherit methods from Object.prototype, or has its hasOwnProperty method overridden/deleted.

example:

const JSONbig = require('json-bigint')  
 <JsonViewer  
      bigNumber={BigNumber}  
      src={{a: JSONbig.parse('{"c": 900719977777777999999999123}')}}  
      ...  

// following code is to make a best guess at
// the type for a variable being submitted.

// we are working with a serialized data representation
input = input.trim()
try {
input = JSON.stringify(JSON.parse(input))
input = structuredClone(input)
Copy link
Member

Choose a reason for hiding this comment

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

great

@Kikobeats Kikobeats self-requested a review February 12, 2025 08:58
@Kikobeats Kikobeats merged commit 9499d73 into microlinkhq:master Feb 12, 2025
3 checks passed
@Suntgr
Copy link
Contributor Author

Suntgr commented Feb 14, 2025

@Suntgr sorry I just reviewed right now, It seems great however test are broken in the main repo and that is blocking this.

Fixing tests should be addressed first, appreciate if you can open a new PR for that.

Thank you for reviewing and merging the PR! I noticed your comment about the broken tests. Since the PR has been merged, would you still like me to create a new PR to address the test issues?

@Kikobeats
Copy link
Member

@Suntgr that would be great.

I downgraded tests to run using node 20 58b6012

but it could be better to use node 22 🙂

@Suntgr
Copy link
Contributor Author

Suntgr commented Feb 14, 2025

@Suntgr that would be great.

I downgraded tests to run using node 20 58b6012

but it could be better to use node 22 🙂

I'll take a look at the tests under Node 22 environment and try to fix any issues. I'll create a new PR once I have a solution.

@Suntgr
Copy link
Contributor Author

Suntgr commented Feb 15, 2025

@Suntgr that would be great.

I downgraded tests to run using node 20 58b6012

but it could be better to use node 22 🙂

Done #62

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