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

Should add test to ensure correct order of search list #64

Closed jacobq closed 5 years ago

jacobq commented 5 years ago

The main search loop here assumes that allTargets is in order: https://github.com/lgeiger/node-abi/blob/9c5acd11ae297c4eabb29c531334dec4b09ebdf3/index.js#L22-L27

By construction (deprecatedTargets first, ..., futureTargets last, and each of those lists written in increasing order) this should be true. However, the test suite does not check this, and it's easy for bugs to creep in. For example, 


var supportedTargets = [
  // ...
  {runtime: 'electron', target: '5.0.0', abi: '70', lts: false}
]

var futureTargets = [
  {runtime: 'electron', target: '5.0.0-beta.0', abi: '68', lts: false}
]

then attempting to run getAbi('5.0.0', 'electron') throws: 

Error: Could not detect abi for version 5.0.0 and runtime electron.  Updating "node-abi" might help solve this issue if it is a new release of electron
    at Object.getAbi (/path/node-abi/index.js:41:9)

This is because semver.lte('5.0.0-beta.0','5.0.0') === true but 5.0.0 came before 5.0.0-beta.0 in the list being searched.

See https://github.com/lgeiger/node-abi/pull/62#discussion_r278280059

jacobq commented 5 years ago

Oops, haha, I just saw that there actually is a test for this -- my bad!

https://github.com/lgeiger/node-abi/blob/1e324fb48e523d9e2327cd1ac622879a6cf1286b/test/index.js#L139-L151