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

Allow replacing JSON.stringify #4450

Open
joshkel opened this issue Jun 1, 2023 · 4 comments
Open

Allow replacing JSON.stringify #4450

joshkel opened this issue Jun 1, 2023 · 4 comments
Labels
feature New functionality or improvement

Comments

@joshkel
Copy link
Contributor

joshkel commented Jun 1, 2023

Support plan

  • is this issue currently blocking your project? (yes/no): No
  • is this issue affecting a production system? (yes/no): Yes

Context

  • node version: 18.16.0
  • module version: 21.3.2
  • environment (e.g. node, browser, native): Node
  • used with (e.g. hapi application, another framework, standalone, ...): Hapi application
  • any other relevant information:

What problem are you trying to solve?

In performance tests, we've found that calling JSON.stringify on large responses is a significant bottleneck. We're experimenting with alternatives such as fast-json-stringify.

Do you have a new or modified API suggestion to solve the problem?

I saw in #3014 that there was a brief discussion about using an alternative to JSON.stringify. I completely understand the desire expressed there to not add dependencies to the Hapi core, but it like there could be some value in adding an option to use a function other than JSON.stringify, so that users can use alternatives themselves if they want.

Suggested API: Add route.options.stringify or route.options.jsonStringify, as a function that takes an arbitrary object and returns a string. If this option is present, and if a response is a normal (i.e., success, not Boom) response of 'plain' type (such that it would normally go through JSON.stringify), then it's invoked instead of JSON.stringify, and route.options.json is ignored.

(Ignoring this new option for Boom error responses simplifies usage with alternatives like fast-json-stringify, which expect to know the schema of the object(s) they're used with.)

@joshkel joshkel added the feature New functionality or improvement label Jun 1, 2023
@Marsup
Copy link
Contributor

Marsup commented Jun 6, 2023

I know it's how some other frameworks deal with it, but I'm questioning whether it's even a good practice that should be encouraged. Shouldn't large payloads be streamed instead of fully serialized?

@kanongil
Copy link
Contributor

kanongil commented Jun 6, 2023

Yeah, at the point where you have a performance issue with the built-in handling, you might be better served with other approaches.

If someone wants to make a generic solution, I expect a new plugin can be made that exposes a dedicated (possibly streaming enabled) h.json() handler. Either using pluggable stringifiers, or directly using fast-json-stringify or similar.

FYI, streaming is probably not viable in a generic sense, but might be viable for specific scenarios. Eg. a for long array of objects.

@devinivy
Copy link
Member

devinivy commented Jun 6, 2023

It's a great idea to have a straightforward way of using fast-json-stringify with hapi. I agree with Gil— experimentation around this is what the plugin system is so useful for, and I think the ideal way of handling this would be to implement a custom response type h.json() similar to how vision's h.view() or inert's h.file() work. This would also have a straightforward way of hooking into settings on the route config, which seems useful in the case of fast-json-stringify which requires a json schema per output type.

@alexey-sh
Copy link

It looks like nobody uses hapi in heavy apps(because it doesn't provide this simple perf tuning). So maybe it makes sense to focus on simplicity and close this issue as 'won't fix'. Just to be a developer friendly and not to give false hope that it will ever be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

5 participants