Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: verify fails if node not installed #167

Merged
merged 20 commits into from
Sep 24, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"@salesforce/prettier-config": "^0.0.2",
"@salesforce/ts-sinon": "1.3.21",
"@types/shelljs": "^0.8.9",
"@types/sinon-chai": "^3.2.5",
"@typescript-eslint/eslint-plugin": "^4.2.0",
"@typescript-eslint/parser": "^4.2.0",
"chai": "^4.2.0",
Expand All @@ -47,6 +48,7 @@
"pretty-quick": "^3.1.0",
"shx": "0.3.3",
"sinon": "10.0.0",
"sinon-chai": "^3.7.0",
"ts-node": "^10.0.0",
"typescript": "^4.1.3"
},
Expand Down
22 changes: 20 additions & 2 deletions src/lib/npmCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@
*/
/* eslint-disable @typescript-eslint/no-unused-vars */

import { type as osType } from 'os';
Copy link
Contributor Author

@peternhale peternhale Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maggiben why the alias? This confuses old people like me :) I would rather see os.type()

Copy link
Contributor

@RodEsp RodEsp Sep 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peternhale that was me.
Because if you want to do os.type() then you have to bring in all of os with import * as os from 'os';.
So you can do import { type } from 'os'; to only bring in what you need but then calling type() isn't very descriptive either, so I aliased it to osType();

import * as path from 'path';
import * as shelljs from 'shelljs';

import npmRunPath from 'npm-run-path';
import * as shelljs from 'shelljs';

import { SfdxError, fs } from '@salesforce/core';

export type NpmMeta = {
Expand Down Expand Up @@ -102,11 +105,26 @@ export class NpmCommand {
* @private
*/
private static findNode(root: string = undefined): string {
const isExecutable = (filepath: string): boolean => {
if (osType() === 'Windows_NT') return filepath.endsWith('node.exe');

try {
if (filepath.endsWith('node')) {
// This checks if the filepath is executable on Mac or Linux, if it is not it errors.
fs.accessSync(filepath, fs.constants.X_OK);
return true;
}
} catch {
return false;
}
return false;
};

if (root) {
const sfdxBinDirs = NpmCommand.findSfdxBinDirs(root);
if (sfdxBinDirs.length > 0) {
// Find the node executable
const node = shelljs.find(sfdxBinDirs).find((file) => file.includes('node'));
const node = shelljs.find(sfdxBinDirs).filter((file) => isExecutable(file))[0];
if (node) {
return fs.realpathSync(node);
}
Expand Down
42 changes: 36 additions & 6 deletions test/lib/npmCommand.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@
*/

import { fail } from 'assert';
import { expect } from 'chai';
import Sinon = require('sinon');
import * as os from 'os';
import { expect, use as chaiUse } from 'chai';
import * as Sinon from 'sinon';
import * as SinonChai from 'sinon-chai';
import * as shelljs from 'shelljs';
import { stubMethod } from '@salesforce/ts-sinon';
import { fs } from '@salesforce/core';
import { NpmModule } from '../../src/lib/npmCommand';

chaiUse(SinonChai);

const DEFAULT_REGISTRY = 'https://registry.npmjs.org/';
const MODULE_NAME = '@salesforce/plugin-source';
const MODULE_VERSION = '1.0.0';
Expand Down Expand Up @@ -128,6 +132,8 @@ describe('should find the node executable', () => {
let shelljsWhichStub: Sinon.SinonStub;
let existsSyncStub: Sinon.SinonStub;
let realpathSyncStub: Sinon.SinonStub;
let osTypeStub: Sinon.SinonStub;
let accessSyncStub: Sinon.SinonStub;

beforeEach(() => {
sandbox = Sinon.createSandbox();
Expand Down Expand Up @@ -169,6 +175,11 @@ describe('should find the node executable', () => {
expect(filePath).to.be.a('string').and.to.have.length.greaterThan(0);
return true;
});
accessSyncStub = stubMethod(sandbox, fs, 'accessSync').callsFake((filePath: string) => {
expect(filePath).to.be.a('string').and.to.have.length.greaterThan(0);
return undefined;
});
osTypeStub = stubMethod(sandbox, os, 'type').callsFake(() => 'Linux');
});

afterEach(() => {
Expand All @@ -177,10 +188,29 @@ describe('should find the node executable', () => {

it('finds node binary inside sfdx bin folder and runs npm show command', () => {
const npmMetadata = new NpmModule(MODULE_NAME, undefined, __dirname).show(DEFAULT_REGISTRY);
expect(existsSyncStub.callCount).to.equal(2);
expect(shelljsFindStub.callCount).to.equal(1);
expect(realpathSyncStub.callCount).to.equal(1);
expect(shelljsExecStub.callCount).to.equal(1);
expect(accessSyncStub).to.have.been.calledOnce;
expect(existsSyncStub).to.have.been.calledTwice;
expect(osTypeStub).to.have.been.calledOnce;
expect(realpathSyncStub).to.have.been.calledOnce;
expect(shelljsExecStub).to.have.been.calledOnce;
expect(shelljsFindStub).to.have.been.calledOnce;
expect(shelljsExecStub.firstCall.args[0]).to.include(NODE_PATH);
expect(shelljsExecStub.firstCall.args[0]).to.include(`show ${MODULE_NAME}@latest`);
expect(shelljsExecStub.firstCall.args[0]).to.include(`--registry=${DEFAULT_REGISTRY}`);
expect(npmMetadata).to.deep.equal(SHOW_RESULT);
});

it('finds node binary inside sfdx bin folder on windows and runs npm show command', () => {
shelljsFindStub.returns(['C:\\Program Files\\sfdx\\client\\bin\\node.exe']);
osTypeStub.returns('Windows_NT');

const npmMetadata = new NpmModule(MODULE_NAME, undefined, __dirname).show(DEFAULT_REGISTRY);
expect(accessSyncStub).to.not.have.been.called;
expect(existsSyncStub).to.have.been.calledTwice;
expect(osTypeStub).to.have.been.calledOnce;
expect(realpathSyncStub).to.have.been.calledOnce;
expect(shelljsExecStub).to.have.been.calledOnce;
expect(shelljsFindStub).to.have.been.calledOnce;
expect(shelljsExecStub.firstCall.args[0]).to.include(NODE_PATH);
expect(shelljsExecStub.firstCall.args[0]).to.include(`show ${MODULE_NAME}@latest`);
expect(shelljsExecStub.firstCall.args[0]).to.include(`--registry=${DEFAULT_REGISTRY}`);
Expand Down
13 changes: 13 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1050,6 +1050,14 @@
"@types/glob" "*"
"@types/node" "*"

"@types/sinon-chai@^3.2.5":
version "3.2.5"
resolved "https://registry.yarnpkg.com/@types/sinon-chai/-/sinon-chai-3.2.5.tgz#df21ae57b10757da0b26f512145c065f2ad45c48"
integrity sha512-bKQqIpew7mmIGNRlxW6Zli/QVyc3zikpGzCa797B/tRnD9OtHvZ/ts8sYXV+Ilj9u3QRaUEM8xrjgd1gwm1BpQ==
dependencies:
"@types/chai" "*"
"@types/sinon" "*"

"@types/sinon@*", "@types/[email protected]":
version "10.0.0"
resolved "https://registry.yarnpkg.com/@types/sinon/-/sinon-10.0.0.tgz#eecc3847af03d45ffe53d55aaaaf6ecb28b5e584"
Expand Down Expand Up @@ -6913,6 +6921,11 @@ signal-exit@^3.0.0, signal-exit@^3.0.2, signal-exit@^3.0.3:
resolved "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.3.tgz#a1410c2edd8f077b08b4e253c8eacfcaf057461c"
integrity sha512-VUJ49FC8U1OxwZLxIbTTrDvLnf/6TDgxZcK8wxR8zs13xpx7xbG60ndBlhNrFi2EMuFRoeDoJO7wthSLq42EjA==

sinon-chai@^3.7.0:
version "3.7.0"
resolved "https://registry.yarnpkg.com/sinon-chai/-/sinon-chai-3.7.0.tgz#cfb7dec1c50990ed18c153f1840721cf13139783"
integrity sha512-mf5NURdUaSdnatJx3uhoBOrY9dtL19fiOtAdT1Azxg3+lNJFiuN0uzaU3xX1LeAfL17kHQhTAJgpsfhbMJMY2g==

[email protected]:
version "10.0.0"
resolved "https://registry.yarnpkg.com/sinon/-/sinon-10.0.0.tgz#52279f97e35646ff73d23207d0307977c9b81430"
Expand Down