Add "binary.tag" option

[thom-nic]

Separates tag creation from the remote_path option.

Not tested yet but wanted to put it up here for review.

Apologies for the whitespace changes - I can try to fix if it's a problem.

Also I updated the README - I wonder why you were using triple-backticks to denote inline code spans instead of just a single backtick pairs? I've only used triple-backticks for multiline blocks.

Fixes #18

[bchr02]

I was going to ask you to split the above else if statement into two, like so:

else if (!this.package_json.binary.tag && this.package_json.binary.host.substr(0, hostPrefix.length) !== hostPrefix){
  		throw new Error('binary.host in package.json should begin with: "' + hostPrefix + '" when binary.tag is not used.');
  	}
else if (this.package_json.binary.tag && 'string' === typeof this.package_json.binary.tag && this.package_json.binary.host.substr(0, hostPrefix.length) !== hostPrefix){
  		throw new Error('binary.host in package.json should be equal to: "' + hostPrefix + '"');
  	}

but now I am pondering whether it makes sense to try to make this backwards compatible with the old format. What are your thoughts? Regardless, I will be making this a MAJOR version change so I'm not sure there is a point to make it backwards compatible.

[bchr02]

I wonder why you were using triple-backticks

me too 😃

Not tested yet but wanted to put it up here for review.

@thom-nic it's looking good! I just posted #19 (comment) and now I am thinking it makes more sense to remove any extra code that allows for backwards compatibility. I think this will make it easier long term to maintain... What are your thoughts on that? Would you mind making those changes?

[bchr02]

Although I haven't tested your changes yet, I gave it some more thought and I don't understand how the package_json.binary.tag will work in conjunction with node-pre-gyp. It's important to understand that node-pre-gyp-github is designed to work in conjunction with node-pre-gyp. Ultimately node-pre-gyp is what handles the piecing together of the remote URL for locating the binary when someone tries to install a dependent npm module which utilizes node-pre-gyp and node-pre-gyp-github. So, if node-pre-gyp is not designed to be able to add the binary.tag, then this won't work. I went through node-pre-gyp/README and see that only the remote_path property supports versioning and I don't see any mention of support for a binary.tag property. Therefore, I wonder if it makes more sense to do something like this:

"host": "https://github.com",
"remote_path": "/[owner]/[repo]/releases/download/{version}"

instead of this:

"host": "https://github.com/[owner]/[repo]/releases/download/",
"remote_path": "{version}"

or instead of what you are proposing:

"host": "https://github.com",
"remote_path": "/[owner]/[repo]/releases/download/",
"tag": "{version}"

[thom-nic]

Certainly your first suggestion:

"host": "https://github.com",
"remote_path": "/[owner]/[repo]/releases/download/{version}"

works, as that's how one would do it if they were not using node-pre-gyp-github. I figured that would not work for you as you were using remote_path to create a tag for release on github. Which I can understand, someone might want their tag to look like `"tag": "v{version}-RELEASE" or something.

I think the extra tag in the binary section will just be ignored by node-pre-gyp. Of course if node-pre-gyp ever decided they wanted to add a binary.tag we might run into problems but I'm hoping that collision won't happen.

I did quickly try to publish and it worked, so a cursory trial suggests node-pre-gyp doesn't care about the extra property. My test release:
https://github.com/thom-nic/node-serialport/releases/tag/untagged-474015985a3b49d1b32d

But certainly if you can think of a way for node-pre-gyp-github to use the first format, that aligns with standard node-pre-gyp configuration so that would be great. I'm amenable to tweaking things either way.

[thom-nic]

hey @bchr02 do you want to try to go without a tag property or keep it as-is?

[bchr02]

@thom-nic sorry for not responding sooner. I believe we can make it work without the tag property but haven't had time to confirm this. I will try to get this done soon. Thanks.

[bchr02]

@thom-nic I hate to keep you waiting any longer. I have certain things going on right now that are keeping me tied up. Any chance you could try to make the change so we could do it without needing a new property tag? Please let me know. Thanks.

[thom-nic]

Yeah I understand. What's your train of thought here as to how it might work? You could def generate the git tag from package_json.version but then there's no opportunity to do something like tags named v{version}-RELEASE, etc. IDK if anyone's actually doing that. Maybe it's best to enforce tag-version parity or something.

I'll take another look at the fields node-pre-gyp defines, it's been a couple weeks since I last looked at this too.