microsoft / vscode

Visual Studio Code
https://code.visualstudio.com
MIT License
163.19k stars 28.85k forks source link

Asymmetry in Memento API's JSON encoding/decoding #209479

Open hyangah opened 6 months ago

hyangah commented 6 months ago

I expected Memento's get to recover the JSON-stringifyable object but it doesn't always.

Repro: https://github.com/hyangah/vscode-extension-samples/commit/194f02e3418ec293956be62e732a43b33d483fbb

In this repro, I stored a Date object using update, and later attempted to retrieve the value using get<Date>.

    const KEY = 'TEST_TIMESTAMP_KEY';
    let value = context.globalState.get<Date>(KEY);
    if (value === undefined) {
        context.globalState.update(KEY, new Date());
        value = context.globalState.get<Date>(KEY);
    }
    console.log(`TEST_TIMESTAMP_KEY = ${value} type=${typeof value}`);
    if (value === undefined || !(value instanceof Date)) {
        throw Error(`unexpected date - ${value}`);
    }

The first round (immediately storing the value) works as expected, except that the value doesn't match the usual ISO 8601 format (JSON.stringify returns)

Congratulations, your extension "helloworld-sample" is now active!
extensionHostProcess.js:144
"2024-04-03T19:24:17.476Z"
extensionHostProcess.js:144
TEST_TIMESTAMP_KEY = Wed Apr 03 2024 15:24:17 GMT-0400 (Eastern Daylight Time) type=object

Restart the extension. Now this time, the error will be thrown, because context.globalState.get<Date>(KEY) is not a Date object. It's string. I am guessing get failed to parse the persisted JSON string (ISO 8601 format), and silently returned a string object.

Congratulations, your extension "helloworld-sample" is now active!
extensionHostProcess.js:144
"2024-04-03T19:25:06.045Z"
extensionHostProcess.js:144
TEST_TIMESTAMP_KEY = 2024-04-03T19:24:17.478Z type=string
(and crash)

I think this code used to work last year, but recently, we started to notice problems caused by this asymmetry. Did Memento API get changed recently?

bpasero commented 5 months ago

Yeah good catch, I think we should make sure that the value we store is JSON.parse(JSON.stringify...) here:

https://github.com/microsoft/vscode/blob/3117776ea2749881205bce06d753440175a53484/src/vs/workbench/api/common/extHostMemento.ts#L79

So that the returned thing matches what you get when later asking.

bpasero commented 5 months ago

Notes to self: is there more custom revive logic here:

https://github.com/microsoft/vscode/blob/d06e212f827ed29ca39f2e8b46231f4a2191bf70/src/vs/workbench/services/extensions/common/rpcProtocol.ts#L24

That could alter the result once it comes back from storage? If so, can we actually provide the right value before it comes back from the renderer?