lucagez / slow-json-stringify

The slowest stringifier in the known universe. Just kidding, it's the fastest (:
https://github.com/lucagez/slow-json-stringify
MIT License
468 stars 17 forks source link

possible error when property names are duplicate #4

Closed alienzhou closed 5 years ago

alienzhou commented 5 years ago

First , great job!

(version 0.2.1) Defining schema with nested objects might occur an error when there are properties with the same name.

For example,

const stringify = sjs({
  a: {
    b: {
      c: 'string',
    },
  },
  d: {
    c: 'number',
  },
});

stringify({
  a: {
    b: {
      c: 'hello',
    },
  },
  d: {
    c: 42,
  },
});
// expected: {"a":{"b":{"c":"hello"}},"d":{"c":42}} 
// actually: {"a":{"b":{"c":"hello"}},"d":{"c":hello}}

a and d both have a property named c. The two elements in the queue are "c" and they are all pointed to a.b.c.

lucagez commented 5 years ago

Thank you for pointing it out. Didn't noticed it. Yes, that's definitely a bug. This happens because the object that stores the path to reach during stringification is using only the property name as retrieval key. So, you can only have one unique key in that object. This could be changed by storing the paths with a unique id instead of the prop name. At the moment I don't have a solution that does not lead to a significant change of the current implementation. Every suggestion/idea is welcomed!🎉

alienzhou commented 5 years ago

Using an index as the key for arrais and map may help. For now I couldn't find a more simple way too.

Besides, I read the _deepPath() function and find that it only gets the first matched one. As a result, the same property name gets the same path. c(for .a.b.c) and c(for .d.c) both return ['a','b','c']. Maybe add a flag when a key is visited?

lucagez commented 5 years ago

In the end I had to change the implementation slightly. Now, the preparation part is changed pretty significantly, the stringification part is practically the same as before. I referenced the test that confirm that the bug is now fixed.

I ditched completely the function _deepPath and the object containing the paths. I implemented a queue containing every path to the correct target property. During stringification, every path in the queue is inserted in the template. This new implementation led to a significant improvement in performances and a slightly smaller bundle too. See the new benchmarks.

So, thank you very much for filing this issue 🎉🎉