Skip to content

Commit

Permalink
chore(release): Fix hang in the test suite (#3005)
Browse files Browse the repository at this point in the history 
* Check for file existence before `npm install` on deploy, this was hanging up in some test environments
* Prefer absolute paths on the deploy pipeline
* Fix `ng add` stalling if you don't select any options
  • Loading branch information
@jamesdaniels
jamesdaniels authored Oct 8, 2021
1 parent 95edc36 commit bea9d67
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 33 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
"test:typings": "node ./tools/run-typings-test.js",
"test:build": "bash ./test/ng-build/build.sh",
"test:all": "npm run test:node && npm run test:chrome-headless && npm run test:typings && npm run test:build",
"build": "rimraf dist && ttsc -p tsconfig.build.json && node --trace-warnings ./tools/build.js",
"build": "rimraf dist && ttsc -p tsconfig.build.json && node --trace-warnings ./tools/build.js && npm pack ./dist/packages-dist",
"build:jasmine": "tsc -p tsconfig.jasmine.json && cp ./dist/packages-dist/schematics/versions.json ./dist/out-tsc/jasmine/schematics",
"changelog": "conventional-changelog -p angular -i CHANGELOG.md -s -r 1"
},
Expand Down
33 changes: 19 additions & 14 deletions src/schematics/deploy/actions.jasmine.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ login.list = () => Promise.resolve([{ user: { email: 'foo@bar.baz' }}]);
login.add = () => Promise.resolve([{ user: { email: 'foo@bar.baz' }}]);
login.use = () => Promise.resolve('foo@bar.baz');

const workspaceRoot = join('home', 'user');

const initMocks = () => {
fsHost = {
moveSync(_: string, __: string) {
Expand All @@ -37,7 +39,10 @@ const initMocks = () => {
copySync(_: string, __: string) {
},
removeSync(_: string) {
}
},
existsSync(_: string) {
return false;
},
};

firebaseMock = {
Expand Down Expand Up @@ -183,7 +188,7 @@ describe('universal deployment', () => {
await deployToFunction(
firebaseMock,
context,
'/home/user',
workspaceRoot,
STATIC_BUILD_TARGET,
SERVER_BUILD_TARGET,
{ preview: false },
Expand All @@ -196,16 +201,16 @@ describe('universal deployment', () => {
const packageArgs = spy.calls.argsFor(0);
const functionArgs = spy.calls.argsFor(1);

expect(packageArgs[0]).toBe(join('dist', 'package.json'));
expect(functionArgs[0]).toBe(join('dist', 'index.js'));
expect(packageArgs[0]).toBe(join(workspaceRoot, 'dist', 'package.json'));
expect(functionArgs[0]).toBe(join(workspaceRoot, 'dist', 'index.js'));
});

it('should create a firebase function (new)', async () => {
const spy = spyOn(fsHost, 'writeFileSync');
await deployToFunction(
firebaseMock,
context,
'/home/user',
workspaceRoot,
STATIC_BUILD_TARGET,
SERVER_BUILD_TARGET,
{ preview: false, outputPath: join('dist', 'functions') },
Expand All @@ -218,16 +223,16 @@ describe('universal deployment', () => {
const packageArgs = spy.calls.argsFor(0);
const functionArgs = spy.calls.argsFor(1);

expect(packageArgs[0]).toBe(join('dist', 'functions', 'package.json'));
expect(functionArgs[0]).toBe(join('dist', 'functions', 'index.js'));
expect(packageArgs[0]).toBe(join(workspaceRoot, 'dist', 'functions', 'package.json'));
expect(functionArgs[0]).toBe(join(workspaceRoot, 'dist', 'functions', 'index.js'));
});

it('should rename the index.html file in the nested dist', async () => {
const spy = spyOn(fsHost, 'renameSync');
await deployToFunction(
firebaseMock,
context,
'/home/user',
workspaceRoot,
STATIC_BUILD_TARGET,
SERVER_BUILD_TARGET,
{ preview: false },
Expand All @@ -240,8 +245,8 @@ describe('universal deployment', () => {
const packageArgs = spy.calls.argsFor(0);

expect(packageArgs).toEqual([
join('dist', 'dist', 'browser', 'index.html'),
join('dist', 'dist', 'browser', 'index.original.html')
join(workspaceRoot, 'dist', 'dist', 'browser', 'index.html'),
join(workspaceRoot, 'dist', 'dist', 'browser', 'index.original.html')
]);
});

Expand All @@ -250,7 +255,7 @@ describe('universal deployment', () => {
await deployToFunction(
firebaseMock,
context,
'/home/user',
workspaceRoot,
STATIC_BUILD_TARGET,
SERVER_BUILD_TARGET,
{ preview: false, outputPath: join('dist', 'functions') },
Expand All @@ -263,8 +268,8 @@ describe('universal deployment', () => {
const packageArgs = spy.calls.argsFor(0);

expect(packageArgs).toEqual([
join('dist', 'functions', 'dist', 'browser', 'index.html'),
join('dist', 'functions', 'dist', 'browser', 'index.original.html')
join(workspaceRoot, 'dist', 'functions', 'dist', 'browser', 'index.html'),
join(workspaceRoot, 'dist', 'functions', 'dist', 'browser', 'index.original.html')
]);
});

Expand All @@ -273,7 +278,7 @@ describe('universal deployment', () => {
await deployToFunction(
firebaseMock,
context,
'/home/user',
workspaceRoot,
STATIC_BUILD_TARGET,
SERVER_BUILD_TARGET,
{ preview: false },
Expand Down
37 changes: 19 additions & 18 deletions src/schematics/deploy/actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ const defaultFsHost: FSHost = {
renameSync,
copySync,
removeSync,
existsSync,
};

const findPackageVersion = (packageManager: string, name: string) => {
Expand Down Expand Up @@ -176,14 +177,14 @@ export const deployToFunction = async (
);
}

const staticOut = staticBuildOptions.outputPath;
const serverOut = serverBuildOptions.outputPath;
const staticOut = join(workspaceRoot, staticBuildOptions.outputPath);
const serverOut = join(workspaceRoot, serverBuildOptions.outputPath);

const functionsOut = options.outputPath || dirname(serverOut);
const functionsOut = options.outputPath ? join(workspaceRoot, options.outputPath) : dirname(serverOut);
const functionName = options.functionName || DEFAULT_FUNCTION_NAME;

const newStaticOut = join(functionsOut, staticOut);
const newServerOut = join(functionsOut, serverOut);
const newStaticOut = join(functionsOut, staticBuildOptions.outputPath);
const newServerOut = join(functionsOut, serverBuildOptions.outputPath);

// New behavior vs. old
if (options.outputPath) {
Expand All @@ -204,14 +205,15 @@ export const deployToFunction = async (
);
}

const functionsPackageJsonPath = join(functionsOut, 'package.json');
fsHost.writeFileSync(
join(functionsOut, 'package.json'),
functionsPackageJsonPath,
JSON.stringify(packageJson, null, 2)
);

fsHost.writeFileSync(
join(functionsOut, 'index.js'),
defaultFunction(serverOut, options, functionName)
defaultFunction(serverBuildOptions.outputPath, options, functionName)
);

if (!options.prerender) {
Expand All @@ -226,10 +228,10 @@ export const deployToFunction = async (
// tslint:disable-next-line:no-non-null-assertion
const siteTarget = options.target ?? context.target!.project;

try {
execSync(`npm --prefix ${functionsOut} i`);
} catch (e) {
console.warn(e.messsage);
if (fsHost.existsSync(functionsPackageJsonPath)) {
execSync(`npm --prefix ${functionsOut} install`);
} else {
console.error(`No package.json exists at ${functionsOut}`);
}

if (options.preview) {
Expand Down Expand Up @@ -287,15 +289,14 @@ export const deployToCloudRun = async (
);
}

const staticOut = staticBuildOptions.outputPath;
const serverOut = serverBuildOptions.outputPath;
const staticOut = join(workspaceRoot, staticBuildOptions.outputPath);
const serverOut = join(workspaceRoot, serverBuildOptions.outputPath);

// TODO pull these from firebase config
const cloudRunOut = options.outputPath || staticBuildOptions.outputPath.replace('/browser', '/run');
const cloudRunOut = options.outputPath ? join(workspaceRoot, options.outputPath) : join(dirname(serverOut), 'run');
const serviceId = options.functionName || DEFAULT_FUNCTION_NAME;

const newStaticOut = join(cloudRunOut, staticOut);
const newServerOut = join(cloudRunOut, serverOut);
const newStaticOut = join(cloudRunOut, staticBuildOptions.outputPath);
const newServerOut = join(cloudRunOut, serverBuildOptions.outputPath);

// This is needed because in the server output there's a hardcoded dependency on $cwd/dist/browser,
// This assumes that we've deployed our application dist directory and we're running the server
Expand All @@ -305,7 +306,7 @@ export const deployToCloudRun = async (
fsHost.copySync(staticOut, newStaticOut);
fsHost.copySync(serverOut, newServerOut);

const packageJson = getPackageJson(context, workspaceRoot, options, join(serverOut, 'main.js'));
const packageJson = getPackageJson(context, workspaceRoot, options, join(serverBuildOptions.outputPath, 'main.js'));
const nodeVersion = packageJson.engines.node;

if (!satisfies(process.versions.node, nodeVersion.toString())) {
Expand Down
1 change: 1 addition & 0 deletions src/schematics/interfaces.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ export interface FSHost {
renameSync(src: string, dest: string): void;
copySync(src: string, dest: string): void;
removeSync(src: string): void;
existsSync(src: string): boolean;
}

export interface WorkspaceProject {
Expand Down
2 changes: 2 additions & 0 deletions src/schematics/setup/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,8 @@ ${Object.entries(config.sdkConfig).reduce(
});
default: throw(new SchematicsException(`Unimplemented PROJECT_TYPE ${config.projectType}`));
}
} else {
return Promise.resolve();
}
};

Expand Down

0 comments on commit bea9d67

Please sign in to comment.