Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

refactor(experimental): add bigint support to fast-stable-stringify #2014

Conversation

buffalojoec
Copy link
Contributor

This PR adds custom bigint support to the cache key functions used by the
resolvers within DataLoader.

The library used to create these cache keys - fast-stable-stringify - does not
support bigint, so I've added a custom cacheKey function to convert bigint
to string in all parameter objects before handing the object over to
fast-stable-stringify.

This somewhat defeats the "fast" purpose of fast-stable-stringify, but as
mentioned in the function's comments, considering the size of these parameter
objects, the effect should be negligible.

Closes #1986

Copy link
Contributor

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, hang on though. I wonder if this is our chance to fork fast-stable-stringify and just add a case to the switch (typeof val) in there, handle bigints, and get out. Thoughts?

@buffalojoec
Copy link
Contributor Author

Oooh, hang on though. I wonder if this is our chance to fork fast-stable-stringify and just add a case to the switch (typeof val) in there, handle bigints, and get out. Thoughts?

I was close to doing that, tbh. Do we want to just put it in the Web3.js monorepo?

@buffalojoec
Copy link
Contributor Author

In addition to the fact it hasn't had a new version in 6 years, the repository is mostly perf testing. We could probably slim it down quite a bit and add it as a package in Web3.js. Seems like it would need minimal support.

@steveluscher
Copy link
Contributor

do-it.gif

I wonder how fast an object hash would be in JS if you just walked an object depth first, turned everything you find into a byte buffer, and the called crypto.subtle.digest() on the whole thing.

function hashObject(obj) {
  const data = new Uint8Array((function*() {
    // Walk the object in the usual ways...
    Object.keys(obj).forEach(key => {
      const value = obj[key];
      switch (typeof value) {
        case 'string':
          yield* stringToBytes(value);
          break;
        case 'number': {
          const floatArray = new Float64Array(1);
          floatArray[0] = value;
          yield* new Uint8Array(floatArray.buffer);
        /* etc etc etc */
      }
    });
  })());
  const hash = await crypto.subtle.digest('SHA-256', data);
  return bytesToHex(hash);
}

@steveluscher
Copy link
Contributor

It would be async though.

@steveluscher
Copy link
Contributor

Somewhat related (not really though).

GoogleChromeLabs/jsbi#30 (comment)

@lorisleiva
Copy link
Contributor

lorisleiva commented Jan 9, 2024

How about using the battle tested original json-stable-stringify package from which the "fast" one forked from?

It is downloaded over 3 million times a week and was last published 2 months ago as opposed to all the "fast" versions that were published 6 years ago. I'm pretty sure the main package would have had some extra optimisations by now.

@buffalojoec
Copy link
Contributor Author

How about using the battle tested original json-stable-stringify package from which the "fast" one forked from?

It is downloaded over 3 million times a week and was last published 2 months ago as opposed to all the "fast" versions that were published 6 years ago. I'm pretty sure the main package would have had some extra optimisations by now.

This one looks like it might work. I can see support for setting the global BigInt.toJson or defining a replacer.

@buffalojoec buffalojoec closed this Jan 9, 2024
@steveluscher
Copy link
Contributor

I can see support for setting the global BigInt.toJson

Definitely definitely don't modify prototypes. web3.js will break in the first app that protects against monkeypatched builtins.

…or defining a replacer.

A replacer doesn't do what we want. What we want is this:

stringify({foo: 1n}); // {"foo":1n}

The most a replacer will let us do (demo) is this:

stringify({foo: 1n}); // {"foo":"1n"}

…which is a hash collision with this:

stringify({foo: '1n'}); // {"foo":"1n"} 

The whole root of the problem here is that we dgaf about JSON – we just need a hash – but all these libraries have as a goal the production of valid JSON.

@steveluscher
Copy link
Contributor

Also, it's fucking huge. https://bundlephobia.com/package/[email protected]
image

Copy link
Contributor

Because there has been no activity on this PR for 14 days since it was merged, it has been automatically locked. Please open a new issue if it requires a follow up.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[graphql] BigInt variables cause runtime fatal when passed to isFinite()
3 participants