search

electron / node-abi

:turtle: :rocket: Get the Node.js and Electron ABI for a given target and runtime
https://www.npmjs.com/node-abi
MIT License
164 stars 58 forks source link

force ABI update #98

Closed MarshallOfSound closed 3 years ago

vecerek commented 3 years ago

@MarshallOfSound I believe this PR broke master. Although, I don't fully understand yet why.

fnm use 10
yarn test # this fails with the error message seen in the Travis build for node <= v10, succeeds with node 12+
git checkout HEAD~1
yarn test # this succeeds
MarshallOfSound commented 3 years ago

I don't see how that could happen lol, this PR just changed a single line in the registry (through the automated script). Super weird

vecerek commented 3 years ago

There's something weird going on with sorting in node <= 10. I'm referring to these lines: https://github.com/lgeiger/node-abi/blob/master/index.js#L92-L94. If we just removed them, the tests would pass 😄 It's funny that the default order is more correct compared to the "sorted" one.

vecerek commented 3 years ago

@MarshallOfSound https://github.com/lgeiger/node-abi/pull/99

The previous implementation of the comparator function (introduced in #95) could only ever return true/false, i.e. 1/0 but no -1. So, what I think happened was that when 0 was returned, Node attempted to apply a default ordering on the objects based on the values of all their properties. That is why a seemingly innocuous change such as changing an lts property from false to true resulted in a broken order. I guess the order was previously correct only by accident 😄