Fix getVersion returning an undefined version

[bastimeyer]

Fixes #343

This also fixes an issue when defining non-existing versions. Using 0.1000.0 for instance doesn't return a proper error message right now, but instead fails with a "Cannot read property 'linux64' of undefined" error. I'm not sure why the non-existing versions test doesn't work correctly...

Also, setting proper git tags for each release would be great, so they don't have to be referred by their commit id (either for comments here or for targeting a specific version with npm). 😃

Thanks!

[adam-lynch]

Fixes #343

Isn't the cause of the issue that they don't have Node >= 4? We've dependencies that require that now.

Using 0.1000.0 for instance doesn't return a proper error message right now, but instead fails with a "Cannot read property 'linux64' of undefined" error. I'm not sure why the non-existing versions test doesn't work correctly...

Hmm yeah I definitely thought this was ok. Are you on Node >= 4?

Also, setting proper git tags for each release would be great, so they don't have to be referred by their commit id (either for comments here or for targeting a specific version with npm). 😃

I could use GitHub releases which would do this for me 👍

[bastimeyer]

Are you on Node >= 4?

$ uname -sr; node -v; npm -v
Linux 4.6.4-1-ARCH
v6.3.1
3.10.5

Isn't the cause of the issue that they don't have Node >= 4? We've dependencies that require that now.

I don't know what's happening on <4.0.0, but it didn't work for me either, so...

There are also a couple of deps that return a warning by npm and should be upgraded:

$ npm install nw-builder
npm WARN deprecated [email protected]: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm WARN deprecated [email protected]: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue

I could use GitHub releases which would do this for me 👍

Or just set the tag locally and push it 😸
I think you can also set up travis ci to automatically deploy to npm when pushing a new tag.

[adam-lynch]

OK I might've jumped to a conclusion there. They're seeing [TypeError: undefined is not a function] and I assumed that was that .find doesn't exist before node 4. This would make sense if using _.find instead prevents the error from happening for them.

I'm sorry but I'm finding it hard to see why this change is needed. The existing code should work fine right? From testing in my browser's DevTools, this returns undefined:

[{version: 1},{version: 2}].find(function(obj){
  if(obj.version === 3){
    return obj;
  }
});

Whereas this returns {version: 2}:

[{version: 1},{version: 2}].find(function(obj){
  if(obj.version === 2){
    return obj;
  }
});

[bastimeyer]

Well, I don't know exactly why it's not working correctly. I have just debugged this in production again.

The callback in versions.find is not being called once, even though versions is a normal array with all the content from the versions manifest.
This returns true though:
Array.isArray(versions) && versions.length >= 1 && versions.find === [].find...

Btw, Array.prototype.find's callback should return a boolean, yours does return an object or undefined, which also works I guess (yeah, I know, I didn't change it in my PR)...

After calling .find, version always is -1 (remember that !!-1 === true), so the intended error message will never be shown and the promise chain will fail somewhere else.

When I change it to lodash's find method, it works. Even changing it to versions.findIndex (and altering the rest of the code) works, which is actually a bit cleaner. This should be done instead in case you decide to merge this, just let me know.

[adam-lynch]

OK yeah I'm a bit perplexed. Sure update it to use versions.findIndex I guess so and I'll include it in the next release.

[bastimeyer]

Just force-pushed the new commit. Should be good2go...

I still don't know what's going on here. This must either be something nodejs related, one of the deps replacing Array.prototype.find with a custom broken implementation or maybe something else.

[adam-lynch]

Yeah, weird! Thanks anyway

[bastimeyer]

I found the cause of the issue of [].find not working correctly.
I'm using grunt for building my application and some of my other dependencies (eg. "q-io") which are being loaded together with grunt are using the "collections" npm module as another dependency.
That's where this issue is coming from: montagejs/collections#139

Although it's nw-builder unrelated, I still think the PR/merge was appropriate in order to solve this issue.

[adam-lynch]

OK