mokkabonna / json-schema-merge-allof

Simplify your schema by combining allOf
98 stars 23 forks source link

What is version 0.7.0? #10

Closed epicfaace closed 4 years ago

epicfaace commented 4 years ago

@mokkabonna I noticed that version 0.7.0 was published to npm 3 months ago.

However, I don't see any 0.7.0 tag in the github source repo -- so which version of the code does 0.7.0 refer to?

jedrichards commented 4 years ago

Whatever it was completely broke my schema scripts, no allOf getting merged on 0.7.0 🤔

mokkabonna commented 4 years ago

Sorry for not tagging

Pushed the tag now: https://github.com/mokkabonna/json-schema-merge-allof/tree/v0.7.0

Was after this PR https://github.com/mokkabonna/json-schema-merge-allof/pull/8/files

Now I am making sure I don't alter the incoming schemas. But rather copying before I start altering.

@jedrichards could you be more specific? All tests do pass so I am really surprised by that. Also I am using 0.7.0 myself at work and that works as always.

What node are you using? I am using the spread operator in object/array, but has been supported by node 8.3 AFAIK.

jedrichards commented 4 years ago

@mokkabonna I'm using Node 13.8.0. Here's the point in our code where we're using json-schema-merge-allof:

https://github.com/sketch-hq/sketch-file-format/blob/master/src/assemble.ts#L225

I haven't had time to dig any deeper, but doing nothing aside from upgrading from 0.6.0 to 0.7.0 results in the schemas we're building still containing unmerged allOf properties.

I'll report back here if I get some time to look into this.

mokkabonna commented 4 years ago

I see you are relying on the mutation of output. The input schema is no longer mutated, as that can lead to some nasty bugs. Just keep the return value of compare.

var merged = mergeAllOf(output, { ignoreAdditionalProperties: true })

I considered that a bug tbh, so didn't announce that any further. Classic example of a bug becoming a feature :)

jedrichards commented 4 years ago

@mokkabonna Ahh, that probably explains it! Yep that is definitely an improvement of the api