From f7177f4e8e0c11b27019f9043b49c864468c3a06 Mon Sep 17 00:00:00 2001 From: Kacper Wiszczuk Date: Sun, 17 Mar 2019 10:30:57 +0100 Subject: [PATCH] Refactor: Rework PackageManager implementation (#243) 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 --- packages/cli/src/cliEntry.js | 3 + packages/cli/src/commands/init/init.js | 11 +-- packages/cli/src/commands/install/install.js | 4 +- .../cli/src/commands/install/uninstall.js | 4 +- .../upgrade/__tests__/upgrade.test.js | 12 ++- packages/cli/src/commands/upgrade/upgrade.js | 7 +- packages/cli/src/tools/PackageManager.js | 81 +++++++++---------- .../tools/__tests__/PackageManager-test.js | 66 +++++++-------- packages/cli/src/tools/generator/templates.js | 15 ++-- packages/cli/src/tools/yarn.js | 9 +-- 10 files changed, 94 insertions(+), 118 deletions(-) diff --git a/packages/cli/src/cliEntry.js b/packages/cli/src/cliEntry.js index fab22bb7c..e7779fca4 100644 --- a/packages/cli/src/cliEntry.js +++ b/packages/cli/src/cliEntry.js @@ -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 @@ -189,6 +190,8 @@ async function setupAndRun() { root, }; + setProjectDir(ctx.root); + const commands = getCommands(ctx.root); commands.forEach(command => addCommand(command, ctx)); diff --git a/packages/cli/src/commands/init/init.js b/packages/cli/src/commands/init/init.js index d663dc617..c248a7c43 100644 --- a/packages/cli/src/commands/init/init.js +++ b/packages/cli/src/commands/init/init.js @@ -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'; /** @@ -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, @@ -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', diff --git a/packages/cli/src/commands/install/install.js b/packages/cli/src/commands/install/install.js index cb514c606..bd9097a0c 100644 --- a/packages/cli/src/commands/install/install.js +++ b/packages/cli/src/commands/install/install.js @@ -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, 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}); diff --git a/packages/cli/src/commands/install/uninstall.js b/packages/cli/src/commands/install/uninstall.js index 4300387b1..654c594a8 100644 --- a/packages/cli/src/commands/install/uninstall.js +++ b/packages/cli/src/commands/install/uninstall.js @@ -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, ctx: ContextT) { @@ -19,7 +19,7 @@ async function uninstall(args: Array, 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}"`); } diff --git a/packages/cli/src/commands/upgrade/__tests__/upgrade.test.js b/packages/cli/src/commands/upgrade/__tests__/upgrade.test.js index 2730bb013..2935afcd0 100644 --- a/packages/cli/src/commands/upgrade/__tests__/upgrade.test.js +++ b/packages/cli/src/commands/upgrade/__tests__/upgrade.test.js @@ -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')), diff --git a/packages/cli/src/commands/upgrade/upgrade.js b/packages/cli/src/commands/upgrade/upgrade.js index d49a1f31b..b30dbf6f5 100644 --- a/packages/cli/src/commands/upgrade/upgrade.js +++ b/packages/cli/src/commands/upgrade/upgrade.js @@ -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'; @@ -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']); diff --git a/packages/cli/src/tools/PackageManager.js b/packages/cli/src/tools/PackageManager.js index bcc8de4a4..b1426e62d 100644 --- a/packages/cli/src/tools/PackageManager.js +++ b/packages/cli/src/tools/PackageManager.js @@ -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, 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) { - 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, options?: Options) { + return shouldUseYarn(options) + ? executeCommand(`yarn add ${packageNames.join(' ')}`, options) + : executeCommand( + `npm install ${packageNames.join(' ')} --save --save-exact`, + options, + ); +} - uninstall(packageNames: Array) { - return this.shouldCallYarn() - ? this.executeCommand(`yarn remove ${packageNames.join(' ')}`) - : this.executeCommand(`npm uninstall ${packageNames.join(' ')} --save`); - } +export function installDev(packageNames: Array, 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, options?: Options) { + return shouldUseYarn(options) + ? executeCommand(`yarn remove ${packageNames.join(' ')}`, options) + : executeCommand(`npm uninstall ${packageNames.join(' ')} --save`, options); } diff --git a/packages/cli/src/tools/__tests__/PackageManager-test.js b/packages/cli/src/tools/__tests__/PackageManager-test.js index 97bb96318..92a47c24b 100644 --- a/packages/cli/src/tools/__tests__/PackageManager-test.js +++ b/packages/cli/src/tools/__tests__/PackageManager-test.js @@ -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(() => {}); @@ -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', @@ -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', @@ -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', @@ -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', @@ -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', @@ -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', @@ -99,9 +77,7 @@ 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', @@ -109,14 +85,28 @@ it('should use npm if yarn is not available', () => { ); }); -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); }); diff --git a/packages/cli/src/tools/generator/templates.js b/packages/cli/src/tools/generator/templates.js index 85d426e82..b771efa72 100644 --- a/packages/cli/src/tools/generator/templates.js +++ b/packages/cli/src/tools/generator/templates.js @@ -13,7 +13,7 @@ import fs from 'fs'; import path from 'path'; import copyProjectTemplateAndReplace from './copyProjectTemplateAndReplace'; import logger from '../logger'; -import PackageManager from '../PackageManager'; +import * as PackageManager from '../PackageManager'; /** * @param destPath Create the new project at this path. @@ -72,9 +72,8 @@ function createFromRemoteTemplate( // Check if the template exists logger.info(`Fetching template ${installPackage}...`); - const packageManager = new PackageManager({projectDir: destinationRoot}); try { - packageManager.install([installPackage]); + PackageManager.install([installPackage]); const templatePath = path.resolve('node_modules', templateName); copyProjectTemplateAndReplace(templatePath, destPath, newProjectName, { // Every template contains a dummy package.json file included @@ -92,7 +91,7 @@ function createFromRemoteTemplate( } finally { // Clean up the temp files try { - packageManager.uninstall([templateName]); + PackageManager.uninstall([templateName]); } catch (err) { // Not critical but we still want people to know and report // if this the clean up fails. @@ -125,9 +124,7 @@ function installTemplateDependencies(templatePath, destinationRoot) { const dependenciesToInstall = Object.keys(dependencies).map( depName => `${depName}@${dependencies[depName]}`, ); - new PackageManager({projectDir: destinationRoot}).install( - dependenciesToInstall, - ); + PackageManager.install(dependenciesToInstall); logger.info("Linking native dependencies into the project's build files..."); execSync('react-native link', {stdio: 'inherit'}); } @@ -157,9 +154,7 @@ function installTemplateDevDependencies(templatePath, destinationRoot) { const dependenciesToInstall = Object.keys(dependencies).map( depName => `${depName}@${dependencies[depName]}`, ); - new PackageManager({projectDir: destinationRoot}).installDev( - dependenciesToInstall, - ); + PackageManager.installDev(dependenciesToInstall); } export {createProjectFromTemplate}; diff --git a/packages/cli/src/tools/yarn.js b/packages/cli/src/tools/yarn.js index 38317daca..bdefbee58 100644 --- a/packages/cli/src/tools/yarn.js +++ b/packages/cli/src/tools/yarn.js @@ -17,7 +17,7 @@ import logger from './logger'; * Use Yarn if available, it's much faster than the npm client. * Return the version of yarn installed on the system, null if yarn is not available. */ -function getYarnVersionIfAvailable() { +export function getYarnVersionIfAvailable() { let yarnVersion; try { // execSync returns a Buffer -> convert to string @@ -48,11 +48,6 @@ function getYarnVersionIfAvailable() { * Let's be safe and not mix yarn and npm in a single project. * @param projectDir e.g. /Users/martin/AwesomeApp */ -function isGlobalCliUsingYarn(projectDir) { +export function isProjectUsingYarn(projectDir) { return fs.existsSync(path.join(projectDir, 'yarn.lock')); } - -export default { - getYarnVersionIfAvailable, - isGlobalCliUsingYarn, -};