sidorares / json-bigint

JSON.parse/stringify with bigints support
MIT License
790 stars 189 forks source link

feat: now allows a custom number parser to be passed as an option. #46

Open neverendingqs opened 3 years ago

neverendingqs commented 3 years ago

the PR looks ok, but I'm a bit worried about growing nimber of options like yours. Maybe we should move to much simple api, for example toNumber() callback that takes number-like string as an input and decides what to return - JS number, BigNumber or something else. If not present the behaviour is automatic ( BigNumber if not all integer digits can be preserved accurately ) ~https://github.com/sidorares/json-bigint/pull/20#issuecomment-346708938

Is this what you had in mind @sidorares?

DonBrinn commented 3 years ago

How would the custom number parser be used to solve the defect in parsing long decimal numbers when using the native BigInt with json-bigint?

neverendingqs commented 3 years ago

How would the custom number parser be used to solve the defect in parsing long decimal numbers when using the native BigInt with json-bigint?

I'm thinking something like this:

const JSONBI = require('json-bigint')({
  numberParser: str => {
    const attempt = Number(str);

    if(str.indexOf('.') >= 0) {
      // Or do something else if the precision of decimal numbers is important
      return attempt;
    }

    if(attempt <= Number.MIN_SAFE_INTEGER || attempt >= Number.MAX_SAFE_INTEGER) {
      return BigInt(str);
    }

    return attempt;
  }
});
neverendingqs commented 3 years ago

@sidorares - bump :)

neverendingqs commented 3 years ago

@sidorares - bump :)

neverendingqs commented 3 years ago

@sidorares - bump :). Any concerns with the pull request?