Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

force ABI update #98

Merged
merged 3 commits into from
Nov 17, 2020
Merged

force ABI update #98

merged 3 commits into from
Nov 17, 2020

Conversation

MarshallOfSound
Copy link
Member

No description provided.

@MarshallOfSound MarshallOfSound merged commit 7b787b4 into master Nov 17, 2020
@MarshallOfSound MarshallOfSound deleted the MarshallOfSound-patch-1 branch November 17, 2020 20:58
@vecerek
Copy link
Contributor

vecerek commented Nov 18, 2020

@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
Copy link
Member Author

MarshallOfSound commented Nov 18, 2020

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
Copy link
Contributor

vecerek commented Nov 18, 2020

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
Copy link
Contributor

vecerek commented Nov 18, 2020

@MarshallOfSound #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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants