Skip to content

Commit

Permalink
feat(generic): print a warning if the package manager used is not a k…
Browse files Browse the repository at this point in the history
…nown good version

ISSUES CLOSED: #269
  • Loading branch information
malept committed Aug 6, 2017
1 parent 7963b6d commit a4c36fa
Showing 1 changed file with 41 additions and 4 deletions.
45 changes: 41 additions & 4 deletions src/util/check-system.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,48 @@
import { exec } from 'child_process';
import logSymbols from 'log-symbols';
import semver from 'semver';

export default async () =>
new Promise((resolve) => {
import { hasYarn, yarnOrNpmSpawn } from './yarn-or-npm';

async function checkGitExists() {
return new Promise((resolve) => {
exec('git --version', (err) => {
if (err) return resolve(false);
resolve(true);
});
})
.then(prev => Promise.resolve(prev && semver.gt(process.versions.node, '6.0.0')));
});
}

async function checkNodeVersion() {
return Promise.resolve(semver.gt(process.versions.node, '6.0.0'));
}

const NPM_WHITELISTED_VERSIONS = '^3.0.0 || ^4.0.0 || ~5.1.0 || ~5.2.0 || >= 5.4.0';
const YARN_WHITELISTED_VERSIONS = '0.23.3 || 0.24.6';

This comment has been minimized.

Copy link
@lookfirst

lookfirst Aug 23, 2017

This seems really sad. I'm using brew install yarn which is currently 0.27.5_1, seemingly way ahead of 0.24.6. How can things be so broken with such a new version?

This comment has been minimized.

Copy link
@malept

malept Aug 23, 2017

Author Member

See: #265

This comment has been minimized.

Copy link
@malept

malept Aug 23, 2017

Author Member

Please note that we can add more versions as time goes on. Also, this is only a warning, Forge will still work but you use that version at your own risk.

This comment has been minimized.

Copy link
@lookfirst

lookfirst Aug 24, 2017

@malept Invert it. If there is a known broken version or range, then you should specify that. Otherwise, you're going to be showing whack-a-mole error messages without any real meaning for years on end. Every time there is a new release of yarn. I'm not even on windows and I'm getting penalized for it.

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Aug 24, 2017

Member

@lookfirst We have really wide version ranges for NPM because we know that only a few versions of NPM were broken. Our issue with yarn at the moment is it seems more releases are broken than aren't, this means we have to take a tougher stance against which versions we know are good. When yarn starts stabilizing and we start seeing less "oh it was my version of yarn causing the issue" comments we'll start to open up the version ranges a bit more.

This comment has been minimized.

Copy link
@lookfirst

lookfirst Aug 24, 2017

@MarshallOfSound This is a bad user experience. I'm on a Mac and the issue referenced here is specifically a Windows issue. I don't know what other issues you're talking about, but we should look into them.

Every time I start up electron, I see this confusing message that isn't even formatted well (it should be on the next line, no?). Penalizing users for yarn's bad behavior doesn't seem like a good solution.

Even worse is that yarn is installed with brew on OSX. Unless I then go peg the version in brew, I can't change it. Even worse is that I don't know what a good version is! It only specifies two versions that are really outdated.

If you're going to add a message like this, at least make it user friendly. =(

yarn start
yarn start v0.27.5
$ electron-forge start
⠴ Checking your system⚠ You are using Yarn, but not a known good version. The known versions that work with Electron Forge are: 0.23.3 || 0.24.6
✔ Checking your system
✔ Locating Application
✔ Preparing native dependencies
✔ Launching Application
Done in 8.06s.

This comment has been minimized.

Copy link
@MarshallOfSound

MarshallOfSound Aug 24, 2017

Member

@lookfirst The known good versions are printed in the error message you posted

The known versions that work with Electron Forge are: 0.23.3 || 0.24.6

Yes, it should be on its own line but that's a tiny behavior thing we can easily tweek.

I don't see how warning users of potentially project-breaking issues are "penalizing" them, it could prevent hours if not days of debugging.

As always PR's are welcome to modify any behavior of forge, if you feel there is a better way for us to do this we welcome all suggestions.

This comment has been minimized.

Copy link
@lookfirst

lookfirst Aug 24, 2017

Sorry, I updated my comment.

It is penalizing because it is some big yellow warning message that comes up that doesn't apply to me. I see it every single time that I start up electron when building my application.

I built a simple skeleton. If someone clones that and tries to use it, they are going to see this as well. Now I have to explain it to them.

I'm trying to discuss it with you before spending time to do a PR, but it seems you aren't willing to even work with me on a solution as you don't see the issue. You are totally ok with this sort of UX, which is disappointing.


function versionWarning(packageManager, whitelistedVersions) {
console.warn(
logSymbols.warning,
(`You are using ${packageManager}, but not a known good version. The known ` +
`versions that work with Electron Forge are: ${whitelistedVersions}`).yellow
);
}

async function checkPackageManagerVersion() {
return yarnOrNpmSpawn(['--version'])
.then((version) => {
if (hasYarn()) {
if (!semver.satisfies(version, YARN_WHITELISTED_VERSIONS)) {
versionWarning('Yarn', YARN_WHITELISTED_VERSIONS);
}
} else if (!semver.satisfies(version, NPM_WHITELISTED_VERSIONS)) {
versionWarning('NPM', NPM_WHITELISTED_VERSIONS);
}

return true;
});
}

export default async () =>
(await Promise.all([checkGitExists(), checkNodeVersion(), checkPackageManagerVersion()]))
.every(check => check);

0 comments on commit a4c36fa

Please sign in to comment.