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

Custom JSON parser #513

Closed
wants to merge 2 commits into from
Closed

Custom JSON parser #513

wants to merge 2 commits into from

Conversation

vbelius
Copy link

@vbelius vbelius commented Feb 9, 2024

To solve the need for custom JSON parses, like in issue [#278] and issue [#478], I've added an option for a custom JSON parser. This is a simpler implementation with more limited scope than the stale [#282] PR.

I was a bit unsure on how to best handle the custom error handling/normalization built into the existing JSON parser. I opted for skipping this when supplied with a custom parser, to allow custom parsers to throw their own errors without manipulation, mostly since the normalizeJsonSyntaxError method removes all keys except message and stack, which might be set on custom and errors and might be useful for those using custom parsers.

      // Only normalize errors if using default parser
      // Custom parsers might throw custom errors
      if (parser === JSON.parse) {
        throw normalizeJsonSyntaxError(e, {
          message: e.message,
          stack: e.stack
        })
      } else {
        throw e
      }

What do you think @dougwilson? Do you have any other feedback that might help get this merged? Thank you.

PS. My personal motivation to implement this feature is to enable the use of json-bigint for parsing json.

@dougwilson
Copy link
Contributor

Hello, and while your PR is appreciated, the intention is that we do not want to add a parser option to eqch and every type, thus having instead a single generic body parser where you can define a paerser that would do anything you want for any type. I'm sorry if you missed that somewhere, as we have closed these prior.

@dougwilson dougwilson closed this Feb 9, 2024
@vbelius
Copy link
Author

vbelius commented Feb 9, 2024

@dougwilson Could I argue that since it's been 7 years without any progress on this issue, and 10 years without any progress on #22, wouldn't an intermediary solution be more beneficial to no solution? It's a very simple PR, not adding lots of code or complexity to the project. It could also be argued for us casual JSON users with, special needs, that it would easier to provide a custom option rather than implement a full custom parser?

I hope you find my contemplations in good spirit, thank you for all your awesome OSS work :)

@dougwilson
Copy link
Contributor

Hello, I would argue perhaps if you like to help, please help make the generic parser... This is adding complexity to the JSON parser that is unnecessary, just think how you are adding the different code paths to avoid the JSON.parse error munging and making the json parser now work two different ways.

I can explain the gap in development, but it is not relevant here and unrelated to trying to add a feature. Please if you are going to contribute to a project, it would be nice to respect the design decisions of the project.

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