perf: stringify diff objects only once#10276
Conversation
✅ Deploy Preview for vitest-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| plugins: PLUGINS, | ||
| } satisfies PrettyFormatOptions | ||
|
|
||
| export interface StringifiedMemory { |
There was a problem hiding this comment.
I initially just wanted to use WeakMap, but we are cloning and reorganizing (set/map) objects, so it's easier to just have an object that sets it
hi-ogawa
left a comment
hi-ogawa
left a comment
There was a problem hiding this comment.
The idea makes sense to me, but is it important to stash the result through memorize?
Naively thinking, can we extend diff to return something like { result: string, actual?: string, expected?: string }? I'd get stashing could be simpler if diff is recursing beast, but doesn't look like so.
It could be just minor nit, but just asking.
| interface Memorize { | ||
| (pointer: 'expected' | 'actual', stringifiedValue: string): string | ||
| } |
There was a problem hiding this comment.
Ah, I think what I felt odd is that memorize interface isn't even explicitly "stashing the result as argument" API.
So my question is, can this be diff(..., memory?: StringifiedMemory) directly instead of stashing through calling memorize: Memorize?
There was a problem hiding this comment.
It could, but it’s just less code with a util. It’s ugly to have val = string after every stringify call 😬
There was a problem hiding this comment.
I get the compactness but it feels an unnecessary indirection for my brain. not blocking though.
We could return different result, but I think the approach in pr is just less code to implement, but it does rely a lot on mutating state which, I admit, is hard to follow |
| interface Memorize { | ||
| (pointer: 'expected' | 'actual', stringifiedValue: string): string | ||
| } |
There was a problem hiding this comment.
I get the compactness but it feels an unnecessary indirection for my brain. not blocking though.
2 participants
Description
Resolves vitest-dev/vscode#781
This PR does two things:
diffOptionsin those properties now (this causes the snapshot in tests to be big)