Skip to content

Commit

Permalink
Refactor: Rework PackageManager implementation (#243)
Browse files Browse the repository at this point in the history
Summary:
---------

Removed nasty `class`, and replaced it with grouped methods.

`projectDir` is always the same in `cli` lifecycle, so we set it once on the start. Based on the presence of `yarn.lock` we decide either to use yarn or not to use it. `preferYarn` is overriding this behavior, and can later be used in `init` command which lacks correct `projectDir`.

Test Plan:
----------

- [x] - Unit tests passing
- [x] - Manual testing
  • Loading branch information
Esemesek authored and thymikee committed Mar 17, 2019
1 parent 79f7d4e commit f7177f4
Show file tree
Hide file tree
Showing 10 changed files with 94 additions and 118 deletions.
3 changes: 3 additions & 0 deletions packages/cli/src/cliEntry.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import init from './commands/init/init';
import assertRequiredOptions from './tools/assertRequiredOptions';
import logger from './tools/logger';
import findPlugins from './tools/findPlugins';
import {setProjectDir} from './tools/PackageManager';
import pkgJson from '../package.json';

commander
Expand Down Expand Up @@ -189,6 +190,8 @@ async function setupAndRun() {
root,
};

setProjectDir(ctx.root);

const commands = getCommands(ctx.root);

commands.forEach(command => addCommand(command, ctx));
Expand Down
11 changes: 3 additions & 8 deletions packages/cli/src/commands/init/init.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import path from 'path';
import process from 'process';
import printRunInstructions from '../../tools/generator/printRunInstructions';
import {createProjectFromTemplate} from '../../tools/generator/templates';
import PackageManager from '../../tools/PackageManager';
import * as PackageManager from '../../tools/PackageManager';
import logger from '../../tools/logger';

/**
Expand Down Expand Up @@ -52,11 +52,6 @@ function generateProject(destinationRoot, newProjectName, options) {
const pkgJson = require('react-native/package.json');
const reactVersion = pkgJson.peerDependencies.react;

const packageManager = new PackageManager({
projectDir: destinationRoot,
forceNpm: options.npm,
});

createProjectFromTemplate(
destinationRoot,
newProjectName,
Expand All @@ -65,10 +60,10 @@ function generateProject(destinationRoot, newProjectName, options) {
);

logger.info('Adding required dependencies');
packageManager.install([`react@${reactVersion}`]);
PackageManager.install([`react@${reactVersion}`]);

logger.info('Adding required dev dependencies');
packageManager.installDev([
PackageManager.installDev([
'@babel/core',
'@babel/runtime',
'jest',
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/commands/install/install.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@

import type {ContextT} from '../../tools/types.flow';
import logger from '../../tools/logger';
import PackageManager from '../../tools/PackageManager';
import * as PackageManager from '../../tools/PackageManager';
import link from '../link/link';

async function install(args: Array<string>, ctx: ContextT) {
const name = args[0];

logger.info(`Installing "${name}"...`);
new PackageManager({projectDir: ctx.root}).install([name]);
PackageManager.install([name]);

logger.info(`Linking "${name}"...`);
await link.func([name], ctx, {platforms: undefined});
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/commands/install/uninstall.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

import type {ContextT} from '../../tools/types.flow';
import logger from '../../tools/logger';
import PackageManager from '../../tools/PackageManager';
import * as PackageManager from '../../tools/PackageManager';
import link from '../link/unlink';

async function uninstall(args: Array<string>, ctx: ContextT) {
Expand All @@ -19,7 +19,7 @@ async function uninstall(args: Array<string>, ctx: ContextT) {
await link.func([name], ctx);

logger.info(`Uninstalling "${name}"...`);
new PackageManager({projectDir: ctx.root}).uninstall([name]);
PackageManager.uninstall([name]);

logger.success(`Successfully uninstalled and unlinked "${name}"`);
}
Expand Down
12 changes: 5 additions & 7 deletions packages/cli/src/commands/upgrade/__tests__/upgrade.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,11 @@ jest.mock(
() => ({name: 'TestApp', dependencies: {'react-native': '^0.57.8'}}),
{virtual: true},
);
jest.mock('../../../tools/PackageManager', () =>
jest.fn(() => ({
install: args => {
mockPushLog('$ yarn add', ...args);
},
})),
);
jest.mock('../../../tools/PackageManager', () => ({
install: args => {
mockPushLog('$ yarn add', ...args);
},
}));
jest.mock('../helpers', () => ({
...jest.requireActual('../helpers'),
fetch: jest.fn(() => Promise.resolve('patch')),
Expand Down
7 changes: 4 additions & 3 deletions packages/cli/src/commands/upgrade/upgrade.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import semver from 'semver';
import execa from 'execa';
import type {ContextT} from '../../tools/types.flow';
import logger from '../../tools/logger';
import PackageManager from '../../tools/PackageManager';
import * as PackageManager from '../../tools/PackageManager';
import {fetch} from './helpers';
import legacyUpgrade from './legacyUpgrade';

Expand Down Expand Up @@ -109,12 +109,13 @@ const installDeps = async (newVersion, projectDir) => {
`Installing "react-native@${newVersion}" and its peer dependencies...`,
);
const peerDeps = await getRNPeerDeps(newVersion);
const pm = new PackageManager({projectDir});
const deps = [
`react-native@${newVersion}`,
...Object.keys(peerDeps).map(module => `${module}@${peerDeps[module]}`),
];
await pm.install(deps, {silent: true});
PackageManager.install(deps, {
silent: true,
});
await execa('git', ['add', 'package.json']);
try {
await execa('git', ['add', 'yarn.lock']);
Expand Down
81 changes: 40 additions & 41 deletions packages/cli/src/tools/PackageManager.js
Original file line number Diff line number Diff line change
@@ -1,53 +1,52 @@
// @flow
import {execSync} from 'child_process';
import yarn from './yarn';
import {getYarnVersionIfAvailable, isProjectUsingYarn} from './yarn';

type PackageManagerOptions = {
forceNpm?: boolean,
projectDir: string,
};
type Options = {|
preferYarn?: boolean,
silent?: boolean,
|};

export default class PackageManager {
options: PackageManagerOptions;
let projectDir;

constructor(options: PackageManagerOptions) {
this.options = options;
}
function executeCommand(command: string, options?: Options) {
return execSync(command, {
stdio: options && options.silent ? 'pipe' : 'inherit',
});
}

executeCommand(command: string, options?: {silent: boolean}) {
return execSync(command, {
stdio: options && options.silent ? 'pipe' : 'inherit',
});
function shouldUseYarn(options?: Options) {
if (options && options.preferYarn !== undefined) {
return options.preferYarn && getYarnVersionIfAvailable();
}

shouldCallYarn() {
return (
!this.options.forceNpm &&
yarn.getYarnVersionIfAvailable() &&
yarn.isGlobalCliUsingYarn(this.options.projectDir)
);
}
return isProjectUsingYarn(projectDir) && getYarnVersionIfAvailable();
}

install(packageNames: Array<string>, options?: {silent: boolean}) {
return this.shouldCallYarn()
? this.executeCommand(`yarn add ${packageNames.join(' ')}`, options)
: this.executeCommand(
`npm install ${packageNames.join(' ')} --save --save-exact`,
options,
);
}
export function setProjectDir(dir: string) {
projectDir = dir;
}

installDev(packageNames: Array<string>) {
return this.shouldCallYarn()
? this.executeCommand(`yarn add -D ${packageNames.join(' ')}`)
: this.executeCommand(
`npm install ${packageNames.join(' ')} --save-dev --save-exact`,
);
}
export function install(packageNames: Array<string>, options?: Options) {
return shouldUseYarn(options)
? executeCommand(`yarn add ${packageNames.join(' ')}`, options)
: executeCommand(
`npm install ${packageNames.join(' ')} --save --save-exact`,
options,
);
}

uninstall(packageNames: Array<string>) {
return this.shouldCallYarn()
? this.executeCommand(`yarn remove ${packageNames.join(' ')}`)
: this.executeCommand(`npm uninstall ${packageNames.join(' ')} --save`);
}
export function installDev(packageNames: Array<string>, options?: Options) {
return shouldUseYarn(options)
? executeCommand(`yarn add -D ${packageNames.join(' ')}`, options)
: executeCommand(
`npm install ${packageNames.join(' ')} --save-dev --save-exact`,
options,
);
}

export function uninstall(packageNames: Array<string>, options?: Options) {
return shouldUseYarn(options)
? executeCommand(`yarn remove ${packageNames.join(' ')}`, options)
: executeCommand(`npm uninstall ${packageNames.join(' ')} --save`, options);
}
66 changes: 28 additions & 38 deletions packages/cli/src/tools/__tests__/PackageManager-test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// @flow
import ChildProcess from 'child_process';
import PackageManager from '../PackageManager';
import yarn from '../yarn';
import * as PackageManager from '../PackageManager';
import * as yarn from '../yarn';

const PROJECT_DIR = '/project/directory';
const PACKAGES = ['react', 'react-native'];
const EXEC_OPTS = {stdio: 'inherit'};
const PROJECT_ROOT = '/some/dir';

beforeEach(() => {
jest.spyOn(ChildProcess, 'execSync').mockImplementation(() => {});
Expand All @@ -16,13 +16,10 @@ describe('yarn', () => {
jest
.spyOn(yarn, 'getYarnVersionIfAvailable')
.mockImplementation(() => true);
jest.spyOn(yarn, 'isGlobalCliUsingYarn').mockImplementation(() => true);
});

it('should install', () => {
const packageManager = new PackageManager({projectDir: PROJECT_DIR});

packageManager.install(PACKAGES);
PackageManager.install(PACKAGES, {preferYarn: true});

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'yarn add react react-native',
Expand All @@ -31,9 +28,7 @@ describe('yarn', () => {
});

it('should installDev', () => {
const packageManager = new PackageManager({projectDir: PROJECT_DIR});

packageManager.installDev(PACKAGES);
PackageManager.installDev(PACKAGES, {preferYarn: true});

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'yarn add -D react react-native',
Expand All @@ -42,9 +37,7 @@ describe('yarn', () => {
});

it('should uninstall', () => {
const packageManager = new PackageManager({projectDir: PROJECT_DIR});

packageManager.uninstall(PACKAGES);
PackageManager.uninstall(PACKAGES, {preferYarn: true});

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'yarn remove react react-native',
Expand All @@ -55,12 +48,7 @@ describe('yarn', () => {

describe('npm', () => {
it('should install', () => {
const packageManager = new PackageManager({
projectDir: PROJECT_DIR,
forceNpm: true,
});

packageManager.install(PACKAGES);
PackageManager.install(PACKAGES, {preferYarn: false});

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'npm install react react-native --save --save-exact',
Expand All @@ -69,12 +57,7 @@ describe('npm', () => {
});

it('should installDev', () => {
const packageManager = new PackageManager({
projectDir: PROJECT_DIR,
forceNpm: true,
});

packageManager.installDev(PACKAGES);
PackageManager.installDev(PACKAGES, {preferYarn: false});

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'npm install react react-native --save-dev --save-exact',
Expand All @@ -83,12 +66,7 @@ describe('npm', () => {
});

it('should uninstall', () => {
const packageManager = new PackageManager({
projectDir: PROJECT_DIR,
forceNpm: true,
});

packageManager.uninstall(PACKAGES);
PackageManager.uninstall(PACKAGES, {preferYarn: false});

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'npm uninstall react react-native --save',
Expand All @@ -99,24 +77,36 @@ describe('npm', () => {

it('should use npm if yarn is not available', () => {
jest.spyOn(yarn, 'getYarnVersionIfAvailable').mockImplementation(() => false);
const packageManager = new PackageManager({projectDir: PROJECT_DIR});

packageManager.install(PACKAGES);
PackageManager.install(PACKAGES, {preferYarn: true});

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'npm install react react-native --save --save-exact',
EXEC_OPTS,
);
});

it('should use npm if global cli is not using yarn', () => {
jest.spyOn(yarn, 'isGlobalCliUsingYarn').mockImplementation(() => false);
const packageManager = new PackageManager({projectDir: PROJECT_DIR});
it('should use npm if project is not using yarn', () => {
jest.spyOn(yarn, 'isProjectUsingYarn').mockImplementation(() => false);

packageManager.install(PACKAGES);
PackageManager.setProjectDir(PROJECT_ROOT);
PackageManager.install(PACKAGES);

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'npm install react react-native --save --save-exact',
EXEC_OPTS,
);
expect(yarn.isProjectUsingYarn).toHaveBeenCalledWith(PROJECT_ROOT);
});

it('should use yarn if project is using yarn', () => {
jest.spyOn(yarn, 'isProjectUsingYarn').mockImplementation(() => false);

PackageManager.setProjectDir(PROJECT_ROOT);
PackageManager.install(PACKAGES);

expect(ChildProcess.execSync).toHaveBeenCalledWith(
'yarn add react react-native',
EXEC_OPTS,
);
expect(yarn.isProjectUsingYarn).toHaveBeenCalledWith(PROJECT_ROOT);
});
Loading

0 comments on commit f7177f4

Please sign in to comment.