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

feat: create amplify skip prompts for --yes option #507

Merged
merged 12 commits into from
Oct 30, 2023
6 changes: 6 additions & 0 deletions .changeset/metal-tomatoes-check.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'create-amplify': patch
---

1. Create Amplify (temporarily) uses environment variable for Package Manager
2. Create Amplify uses `yes` option to choose the default value for prompts
17 changes: 16 additions & 1 deletion packages/create-amplify/src/get_project_root.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@ import { afterEach, describe, it } from 'node:test';
import assert from 'assert';
import fsp from 'fs/promises';
import path from 'path';
import { getProjectRoot } from './get_project_root.js';
import { AmplifyPrompter } from '@aws-amplify/cli-core';
import { getProjectRoot } from './get_project_root.js';

const originalEnv = process.env;

Expand Down Expand Up @@ -68,4 +68,19 @@ void describe('getProjectRoot', () => {
);
assert.equal(projectRoot, path.resolve(userInput));
});

void it('use default options if `yes`', async (ctx) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

if 'yes' is intended to mean 'default', we should name the flag 'default'. This is a huge source of confusion in the current CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. I almost udpated the PR, but just realized that we can't change it for npm because npm_config_yes is set by --yes.

Do we want to have --default works for all, and --yes only works for npm?

process.env.npm_config_yes = 'false';
process.argv = ['node', 'test.js', '--yes'];
const userInput = 'test';
const fsMkDirSyncMock = ctx.mock.method(fsp, 'mkdir', () => undefined);
ctx.mock.method(fsp, 'stat', () => Promise.reject(new Error()));
ctx.mock.method(AmplifyPrompter, 'input', () => Promise.resolve(userInput));

const projectRoot = await getProjectRoot();

assert.equal(fsMkDirSyncMock.mock.callCount(), 1);
assert.equal(fsMkDirSyncMock.mock.calls[0].arguments[0], process.cwd());
assert.equal(projectRoot, process.cwd());
});
});
9 changes: 8 additions & 1 deletion packages/create-amplify/src/get_project_root.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
import fsp from 'fs/promises';
import path from 'path';
import yargs from 'yargs';
import { AmplifyPrompter } from '@aws-amplify/cli-core';
import { logger } from './logger.js';

/**
* Returns the project root directory.
*/
export const getProjectRoot = async () => {
const useDefault = process.env.npm_config_yes === 'true';
const argv = await yargs(process.argv.slice(2)).options({
yes: {
type: 'boolean',
default: false,
},
}).argv;
const useDefault = process.env.npm_config_yes === 'true' || argv.yes === true;
Copy link
Contributor

Choose a reason for hiding this comment

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

why couple ourselves to npm_config_yes at all? That's a behavior of npm/npx that we shouldn't need to depend on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When create amplify, user are not able to use npm_config_yes for anything else, so it's safe to borrow it for our purpose. Passing --yes is a better DX than passing -- --yes, which is why we chose to use npm_config_yes

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, why have argv.yes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yarn is an exceptional ☹️

const defaultProjectRoot = '.';
let projectRoot: string = useDefault
? defaultProjectRoot
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ void describe('NpmPackageManagerController', () => {
await npmPackageManagerController.installDependencies(['testDep'], 'dev');
assert.deepStrictEqual(execaMock.mock.calls[0].arguments, [
'npm',
['install', 'testDep', '--save-dev'],
['install', 'testDep', '-D'],
{ cwd: 'testPath', stdio: 'inherit' },
]);
});
Expand Down
9 changes: 6 additions & 3 deletions packages/create-amplify/src/npm_package_manager_controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ export class NpmPackageManagerController implements PackageManagerController {
private readonly projectRoot: string,
private readonly execa = _execa
) {}
private readonly executableName = 'npm';
private readonly executableName =
process.env.PACKAGE_MANAGER_EXECUTABLE || 'npm'; // TODO: replace `process.env.PACKAGE_MANAGER_EXECUTABLE` with `getPackageManagerName()` once the test infra is ready.

/**
* Installs the given package names as devDependencies
Expand All @@ -24,9 +25,11 @@ export class NpmPackageManagerController implements PackageManagerController {
packageNames: string[],
type: DependencyType
): Promise<void> => {
const args = ['install'].concat(...packageNames);
const args = [this.executableName === 'yarn' ? 'add' : 'install'].concat(
...packageNames
);
if (type === 'dev') {
args.push('--save-dev');
args.push('-D');
Copy link
Member

Choose a reason for hiding this comment

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

does this work for both ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, -D works for all. --save-dev works for npm, --dev works for yarn

Copy link
Contributor

Choose a reason for hiding this comment

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

we shouldn't be making admissions for yarn in the "npm_package_manager_controller". We need a completely separate controller for yarn support

Copy link
Contributor Author

@0618 0618 Oct 27, 2023

Choose a reason for hiding this comment

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

The command and options of npm, yarn, pnpm, bun are mostly the same, it isn't worth it to have 4 package_manager_controller

Copy link
Contributor

Choose a reason for hiding this comment

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

We should be careful not to create accidental coupling in the name of reducing duplication. Just because two implementations are the same now doesn't mean they will evolve in the same way. There's nothing but convention keeping the args of these tools similar. But they are all separate and will have their own evolutions over time. I still think we should have completely separate controllers for each one even if each controller does 90% of the same stuff as the others.

Consider the case where pnpm updates some flag and we make a change to our uber controller to handle this but it also borks yarn and npm. Vs a separate controller for each where we can update the pnpm controller with confidence that there won't be side-effects for the other controllers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense! It'll be handled in a separated issue #517 before the feature branch poc/package-manager-support merge to main

}
await this.execa(this.executableName, args, {
stdio: 'inherit',
Expand Down
16 changes: 12 additions & 4 deletions packages/create-amplify/src/tsconfig_initializer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export class TsConfigInitializer {
private readonly existsSync = _existsSync,
private readonly execa = _execa
) {}
private readonly executableName =
process.env.PACKAGE_MANAGER_EXECUTABLE || 'npx'; // TODO: replace `process.env.PACKAGE_MANAGER_EXECUTABLE` with `getPackageManagerName()` once the test infra is ready.

/**
* If tsconfig.json already exists, this is a noop. Otherwise, `npx tsc --init` is executed to create a tsconfig.json file
Expand All @@ -27,7 +29,9 @@ export class TsConfigInitializer {
return;
}
this.logger.log(
'No tsconfig.json file found in the current directory. Running `npx tsc --init`...'
`No tsconfig.json file found in the current directory. Running \`${
this.executableName === 'npm' ? 'npx' : this.executableName
} tsc --init\`...`
);

const packageJson = await this.packageJsonReader.readPackageJson();
Expand All @@ -53,20 +57,24 @@ export class TsConfigInitializer {
}

try {
await this.execa('npx', tscArgs, {
await this.execa(this.executableName, tscArgs, {
stdio: 'inherit',
cwd: this.projectRoot,
});
} catch {
throw new Error(
'`npx tsc --init` did not exit successfully. Initialize a valid TypeScript configuration before continuing.'
`\`${
this.executableName === 'npm' ? 'npx' : this.executableName
} tsc --init\` did not exit successfully. Initialize a valid TypeScript configuration before continuing.`
);
}

if (!this.tsConfigJsonExists()) {
// this should only happen if the customer exits out of npx tsc --init before finishing
throw new Error(
'tsconfig.json does not exist after running `npx tsc --init`. Initialize a valid TypeScript configuration before continuing.'
`tsconfig.json does not exist after running \`${
this.executableName === 'npm' ? 'npx' : this.executableName
} tsc --init\`. Initialize a valid TypeScript configuration before continuing.`
);
}
};
Expand Down