-
Notifications
You must be signed in to change notification settings - Fork 473
Allow SSR exchange to use custom serializer #3759
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
base: main
Are you sure you want to change the base?
Conversation
|
|
This has been missing multiple things from our contributing guide We at least need a changeset to be able to merge this, which is a changelog entry file that describes the change for release notes: https://github.com/urql-graphql/urql/blob/main/CONTRIBUTING.md#how-do-i-document-a-change-for-the-changelog However, the PR also contains no description on why this change is important to you or why you decided to contribute this and no corresponding issue: https://github.com/urql-graphql/urql/blob/main/CONTRIBUTING.md#how-do-i-propose-changes API additions without an explanation, even if they're straightforward look often without any reasons as to why they're needed like bloat. In this case, while the change is straightforward, exchanges are pluggable/interchangeable, i.e. you can make your own exchange that supports this behaviour separately from the core library, so we'd need a reason to know whether this would be needed by more users |
|
The use-case is to be able to use more effective methods for serialize/deserialize. I would prefer not to copy the whole exchange just to change these 2 methods. Allowing configuration for these methods in exchange options is a big win with little cost. |
That's self-evident, but also imprecise. This is basically the XY Problem. I understand already that you want this change and what it does, but it's an API addition, so I'd love to understand what the motivation is. Sorry for not clarifying this better. The "why" I'm after isn't "Why did you make this change?", as in describing the API and code change, but instead more like "Why did you need this?", so to go to the root of the change request,
The problem to us here is that that's a compelling and documented API surface. Exchanges are basically customisable extensions and we can't feasibly cover all use-cases with all exchanges all the time. The API of each exchange would balloon to tens of options. That's because exchanges are basically almost pure control flow. The same goes for this one. |
https://github.com/banterfm/graphql-crunch
Our tests showed that by using crunch rather than pure-JSON results in 40-50% size reduction of serialized result.
Improve app performance, by reducing network bytes transferred by SSR server |
|
Two small concerns here:
Re. 1) The types do sufficiently enforce and encourage double serialisation which is desirable, so we can probably ignore this. Re. 2) It may be nice to have a test here, specifically for graphql-crunch so we can keep it working? Lastly, the changeset would be needed, but if push comes to shove I can add it myself to the PR, so no worries |
Summary
Allows SSR exchange to use custom serializer.
Set of changes