bug(>=v10.0.0): subsequent calls to compile() with same schema but different type name reuse first name

[serhalp]

Hi. Thank you for the great library!

v10.0.0 introduces a regression compared with the previous version, 9.1.1. I created a small repro in this repo: https://github.com/serhalp/json-schema-to-typescript-cached-name-bug-repro. I confirmed that this behaves as expected up to version 9.1.1 and does not behave as expected starting on 10.0.0 all the way up to the latest at time of writing (10.1.3). See the instructions in the README to try it out for yourself.

The issue is pretty simple. Given:

const result1 = await compile(schema, "MyType1");
const result2 = await compile(schema, "MyType2");

I would expect the TypeScript interface generated in result1 to be called MyType1 and the interface generated in result2 to be called MyType2. However, with versions 10.0.0 and later, they're both called MyType1.

I imagine there was some sort of caching introduced to improved performance, but it isn't including the type name argument in its key – or something of the such.

[serhalp]

I did a bit of digging and, thanks to the DEBUG env var you all have set up, it was pretty easy to pinpoint the cause of the bug! It's because right here on the first call, the library mutates the passed in schema by defaulting id to a normalized version of the passed in name/filename, and so on the second call it will always ignore the given name/filename and use the previously defaulted name, because of the !schema.id check.

Interestingly, it looks like this may have been introduced by my former colleague over in #298! (I think it's in a roundabout away here... where previously it just so happened that the deep equality check to determine if it was the "root" schema would happen to also return false if the schema had been mutated by setting id previously...? I'm not sure.)

In any case, I would argue that – unless explicitly called out as intentional behavior – it's bad, surprising behavior for libraries to mutate arguments to functions, so I would recommend fixing this by deep cloning the schema as the very first step to compile(). What do you think?

[bcherny]

Hey there -- thanks for the report, and for digging in! This is indeed surprising.

We generally try to prefer mutation over copying, since for large (multi-MB) schemas excessive copying can lead to OOMs.

However, your suggestion -- one deep copy at the beginning, then mutate from there on -- seems like a great idea!

[fo-fo]

Plus one for this, it's very surprising behavior (that I ran into) that compile() mutates its argument.

[bcherny]

Published as part of v11.0.0.