search

bchr02 / node-pre-gyp-github

A node-pre-gyp module which provides the ability to publish to GitHub releases.
MIT License
52 stars 32 forks source link

Add "binary.tag" option #19

Open thom-nic opened 7 years ago

thom-nic commented 7 years ago

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 commented 7 years ago

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 https://github.com/bchr02/node-pre-gyp-github/pull/19#discussion_r96569058 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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

bchr02 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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.