ByzantineFailure / BPM-for-Discord

BPM for Discord's Desktop App. Includes one-click installers, update notifications, and custom script support.
GNU Affero General Public License v3.0
17 stars 8 forks source link

Installer - work with newer versions of Node? #58

Open 00dani opened 8 years ago

00dani commented 8 years ago

Currently if you don't have Node.js v4.2.x installed, the BPM installation script will download a temporary copy of v4.2.4 every time it's run. This approach makes sense if the system version of Node is outdated, which is possible on plenty of distros, but less sense if the system version of Node is newer than v4.2.x.

I'm not entirely sure of what the script actually does once it's into the JS part, but I'd be very surprised if it's incompatible with Node > v4.2.x - indeed, I just tried running node index.js $PWD --ptb on my Mac, which has Node 7, and it appears to have worked just fine.

So, can the version constraint be loosened?

ByzantineFailure commented 8 years ago

Node 4.x is the LTS version with minimal API changes -- that version was specifically selected due to its stability.

I am aware that the installer works for versions 5 and 6 (haven't played with 7 but I trust you when you say it does).

Generally speaking, I'm leery of allowing later versions of node as I don't keep track of changes to the fs APIs per version (which are the only APIs the installer uses via libraries to provide rm -rf and ensureDir functionality). That's really the primary reason.

The secondary reason is I'm really lazy and detecting that the version is greater than or equal to 4 (rather than just equal to 4) is work.

Because, frankly, I don't expect the fs API to change any time soon, if you write up a PR I'll take a look at it. You'll need to make sure it works for the windows version as well, but the logic should be similar to it as well.

ByzantineFailure commented 8 years ago

Note, if you make the changes for windows and don't have an environment to test them in I'll gladly test them for you :) (and try and troubleshoot anything that comes up)

00dani commented 8 years ago

Fair enough! I'll look into writing up a PR when I get the chance - I do have a Windows box available so that shouldn't be an issue. :+1:

1byte2bytes commented 7 years ago

I applied a fix for this issue and verified it worked on my MacBook Air (Sierra 10.12.6) on my fork. Tested both release and PTB. I'd appreciate testing on the Linux version though. I have no idea how to go about the Windows script.

Mac | Mac PTB | Linux

Jibodeah commented 7 years ago

Tested on Linux (Canary client), works fine, finds the local install of node (v6.11.1) and uses it with no problems.

ByzantineFailure commented 7 years ago

@1byte2bytes -- Will get working on a windows script soonish. Theoretically I just have to point it at a new binary and update the version flag. I actually build this with Node 7 now so we should be golden.

ByzantineFailure commented 7 years ago

@1byte2bytes Can you format this into a proper PR? I'll take a glance at it (and try and work out what shell magic you're pulling) and replicate it on the powershell side.

1byte2bytes commented 7 years ago

@ByzantineFailure Yes, and I'll write up an explanation for my magic too.