refactor(experimental): use json-stable-stringify with bigint support for cache keys
#2022
Conversation
…rt for cache keys
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
Merge activity
|
buffalojoec
deleted the
01-09-refactor_experimental_use_json-stable-stringify_with_bigint_support_for_cache_keys
branch
|
Unfortunately, this has to be backed out. #2014 (comment) |
|
|
||
| function replacer(_: any, value: any) { | ||
| if (typeof value === 'bigint') { | ||
| return value.toString() + 'n'; |
There was a problem hiding this comment.
This makes these a hash collision:
{foo: 1n}{foo: '1n'}
| "fast-stable-stringify": "^1.0.0", | ||
| "graphql": "^16.8.0" | ||
| "graphql": "^16.8.0", | ||
| "json-stable-stringify": "^1.1.0" |
There was a problem hiding this comment.
|
Alright, I'll revisit this ASAP. Are we sure rolling our own hasher is the most efficient way to do this? Would we reuse it anywhere else? Steve shared a scratch-pad approach to forking |
|
Since the replacer of Making such a low-level helper asynchronous could bubble up the asynchronousnesss all over the place. |
|
Yeah, I see it as real simple. These stringifiers endeavour to be JSON compatible when that’s the last thing we need. We just need something order-independent, deterministic, and unique. nobody will accept a PR for this. We can however fork |
It would definitely suck. On the other hand we could still make a sync version with a userland implementation of SHA-256. I just don’t know how much slower this would be than the ‘crap out megabytes of strings’ solutions. It would definitely be more space efficient though. Ask me for that story later. |
|
🎉 This PR is included in version 1.89.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
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. |
This PR swaps
fast-stable-stringifyforjson-stable-stringifyfor theresolvers' cache key functions.
Adding
bigintsupport via thereplaceroption solves theBigIntparsingbug reported in #1986.
Closes #1986.