perf(core): remove duplicate JSON.stringify call on data in ssrExchange

[negezor]

Summary

Removes the unnecessary JSON.stringify when serializing, since it will be called below anyway

Set of changes

Removing the data pre-property in exchange/ssr during serialization

[changeset-bot]

🦋 Changeset detected

Latest commit: 7c6fa00

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@urql/core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[JoviDeCroock]

I'd prefer removing the if branch then as JSON.stringify(undefined) still has a result. This PR also needs a changeset.

[negezor]

On the other hand, calling JSON.stringify with undefined is slower
https://jsperf.app/vuwofo

[negezor]

The SerializedResult type allows the data to be a string only or undefined. deserializeResult will return undefined if there is no data.

urql/packages/core/src/exchanges/ssr.ts

Line 154 in 6406fd5

data: result.data ? JSON.parse(result.data) : undefined,

Looking at the implementation closer, I'd like to use something more efficient than JSON.stringify, like devalue. Because I can reduce 2mb JSON to 1.1MB, I just checked this out

[kitten]

Haven't looked at this closely yet, but just to point this out ahead of time, this may necessitate a major, or at least a minor bump. We can't be sure of how this is used and hence we can't guarantee that all apps that have been built ensure that the versions of serialisation are the same in the SSR process and on the client-side.

Going for backwards compatibility here would also work.

[negezor]

It doesn't require any breaking changes, as the result is identical, because we are just doing the same action twice. I have already tested in two applications at my place, one on Nuxt and one on custom SSR.

On the other hand, the changes worth introducing are custom serializers and deserializers. For example something like this:

import { uneval } from 'devalue';
// ...

const ssr = ssrExchange({
    isClient: !import.meta.env.SSR,

    // By default
    // serialize: JSON.stringify,
    // deserialize: JSON.parse,

    // Custom for devalue
    serialize: data => data,
    deserialize: data => data,
});

// ...

if (!import.meta.env.SSR) {
  ssr.restoreData(window.__GRAPHQL__ || {});
}

// ...

if (import.meta.env.SSR) {
  const graphqlState = uneval(ssr.extractData());
  return render('index.html', { graphqlState })
}

This will be backwards compatible for the reason that we will default to JSON methods

[kitten]

Had another look, I agree with the removal of the property entry over the if branch, to avoid empty data being stringified. I don't think custom serializers/deserializers are worth it, since it would only come into play when non-JSON data is preserved, which implies that the ssrExchange would simply have to be re-ordered, since custom deserialization can only occur in other exchanges. Merging